Closed Bug 1402394 Opened 3 years ago Closed 3 years ago

CamelCase all React component files in \devtools\client\shared\components\

Categories

(DevTools :: Framework, enhancement, P3)

enhancement

Tracking

(firefox57 fix-optional, firefox58 fixed)

RESOLVED FIXED
Firefox 58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: pbro, Assigned: dan.epstein, Mentored)

References

Details

(Keywords: good-first-bug)

Attachments

(1 file, 6 obsolete files)

We recently agreed to name our React component file names using the CamelCase convention.

However the files in \devtools\client\shared\components\ do not follow this convention, so let's rename them.

This is an easy fix, but make sure you read the contribution docs before starting: http://docs.firefox-dev.tools/ so that you can build Firefox locally and verify that things still work after your change.

After building, to test that things still work, run Firefox, open DevTools and play with the UI. The components in this folder handle sidebars, toggle buttons, search inputs, etc. So you'll need to make sure everything still works. 
But I suspect the build itself would pick up problems if renaming is incorrect.
Hey Brosset, 

I'm interested in working on this. I have read the contribution docs and already have set my Firefox build environment. 

Thanks.

Dan.
Sure, thanks for your interest. This bug is available for you to work on. 
A few more details to help you get started:

- we want to rename files to CamelCase. So, for instance, notification-box.js would become NotificationBox.js
- the build system we use here requires file names to be listed in moz.build files. So that means you will also need to update all entries in the moz.build file that is in the same directory than the files you rename
- make sure to update the React component files in sub-directories too (splitter, tabs, tree)
- in DevTools we use a module loader system, that means modules can be imported in other modules by using a `require` statement. This statement uses a filename-like path. So if you change, say, notification-box.js to NotificationBox.js, you will also need to search the code for all the places where something requires notification-box and change it. You can see one example here: http://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox.js#1025-1026
Hey I have finished renaming all react files (sub-directories, require statements and in moz.build). I have rebuild Firefox and no issues occur while compiling and after testing the UI buttons work as well. 

It's my first time doing this I would like know what are the next steps? what kind of files do I have to attach (.patch, .diff)? 

Thanks
(In reply to Dan Epstein from comment #3)
> Hey I have finished renaming all react files (sub-directories, require
> statements and in moz.build). I have rebuild Firefox and no issues occur
> while compiling and after testing the UI buttons work as well. 
Sounds great! I'll assign the bug to you so other people know it's already being worked on.
> It's my first time doing this I would like know what are the next steps?
> what kind of files do I have to attach (.patch, .diff)? 
Yes, bugzilla typically works with patch/diff files being added as attachment to bugs. 
Here's some docs about that: http://docs.firefox-dev.tools/contributing/making-prs.html
Also: https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Assignee: nobody → dan.epstein
Status: NEW → ASSIGNED
Attachment #8911995 - Flags: review?(pbrosset)
Attachment #8911995 - Flags: feedback?(pbrosset)
Comment on attachment 8912047 [details] [diff] [review]
CamelCase all React component files in \devtools\client\shared\components\

Sorry about that I have just found an error while rebuilding so here is fix for the unsorted error list for \components\tree moz.build file.
Comment on attachment 8912047 [details] [diff] [review]
CamelCase all React component files in \devtools\client\shared\components\

Review of attachment 8912047 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me. Thanks for this!
Let's also rename the CSS files as noted below. Could you attach a new patch for this?

::: devtools/client/shared/components/moz.build
@@ +18,1 @@
>      'notification-box.css',

For consistency, I think we might want to also CamelCase these CSS files. They have the same name as the JS files next to them, so I think it makes sense to rename them too.

::: devtools/client/shared/components/splitter/moz.build
@@ +9,1 @@
>      'split-box.css',

Same here

::: devtools/client/shared/components/tabs/moz.build
@@ +4,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  DevToolsModules(
>      'tabbar.css',

And here too
Attachment #8912047 - Flags: feedback+
Attachment #8911995 - Attachment is obsolete: true
Attachment #8911995 - Flags: review?(pbrosset)
Attachment #8911995 - Flags: feedback?(pbrosset)
When you upload a new patch, you can mark previous patches as obsolete. This way they don't appear in the list of attachments anymore, and it's easy to see which one is the right one. I've marked your previous patch as obsolete now. Hope I marked the right one!
Attached patch Bug1402394Fix.patch (obsolete) — Splinter Review
Thanks for the feedback! Here is the new patch I have changed the .css\href links files to CamelCase Convention
Attachment #8912047 - Attachment is obsolete: true
Comment on attachment 8912522 [details] [diff] [review]
Bug1402394Fix.patch

Review of attachment 8912522 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, that looks good to me. Let's R+ this patch. I've also pushed this commit to CI so we can make sure things still work. Let's follow the progress of the builds and tests here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=71c9856dd9eb08751600d4f44b60132c8f672bdd
Once all good, we'll ask for this commit to be checked-in mozilla-central.
Oh, I also noticed that your commit was a bit outdated and found one typo. I'll rebase it on top of the latest mozilla-central tip and fix that typo and upload the new patch here in a second.
Thanks for your work!
Attachment #8912522 - Flags: review+
Attached patch Bug1402394Fix.patch (obsolete) — Splinter Review
Attachment #8912522 - Attachment is obsolete: true
Attachment #8912573 - Flags: review+
CI is reporting a lot of test failures. You're changing a lot of file names here, so this was expected, and should be easy to fix.
Here's how I would go about it: run the failing tests locally yourself, look at the logs while the test is failing, and use this information to pinpoint where things are missing.
For instance, I can see this: "Message: Module `resource://devtools/client/shared/components/tree/object-provider.js` is not found at resource://devtools/client/shared/components/tree/object-provider.js"
which means that, somewhere in the code, you forgot to rename an import to ObjectProvider.
There are probably others like this. And CI is a good way to catch them all.
In fact, you can also read the logs on CI directly. To do this:
1 - open https://treeherder.mozilla.org/#/jobs?repo=try&revision=71c9856dd9eb08751600d4f44b60132c8f672bdd
2 - click on one of the orange dtN icons, these are failing devtools (dt) tests
3 - once selected, click on the log icon in the toolbar of the bottom panel (left side)
4 - you should then have a new page that lets you scroll through the logs of the failed test.
Hi Patrick, I've renamed and included the test files in the new patch.
Thank you for your help.
Attachment #8912830 - Flags: review?(pbrosset)
Attached patch Bug1402394Fixes.patch (obsolete) — Splinter Review
Thanks. Looks good. I just found a few more places to replace :)
This one is never ending!
Anyway, I thought I'd just fix them quickly myself instead of going through another round-trip through bugzilla. 
So here's the, hopefully, final patch.

Let's see how that goes on CI: https://treeherder.mozilla.org/#/jobs?repo=try&revision=225d365e6d619a14cc824297bd3636d923e829f3
Attachment #8912573 - Attachment is obsolete: true
Attachment #8912830 - Attachment is obsolete: true
Attachment #8912830 - Flags: review?(pbrosset)
Attachment #8913085 - Flags: review+
DevTools tests seem to be passing just fine now. So let's check this patch in!
Thanks Dan for your work on this.
Keywords: checkin-needed
This has conflicts and will need a rebased patch, unfortunately :(
Flags: needinfo?(dan.epstein)
Keywords: checkin-needed
Thanks Ryan. I'll handle this rebase and push to try again because quite a lot of stuff have changed in the meantime.
Flags: needinfo?(dan.epstein)
Attachment #8913085 - Attachment is obsolete: true
Comment on attachment 8913581 [details]
Bug 1402394 CamelCase all React component files in \devtools\client\shared\components\.

https://reviewboard.mozilla.org/r/184960/#review190052
Attachment #8913581 - Flags: review?(pbrosset) → review+
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/941327d5a29b
CamelCase all React component files in \devtools\client\shared\components\. r=pbro
https://hg.mozilla.org/mozilla-central/rev/941327d5a29b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.