Closed Bug 1254242 Opened 9 years ago Closed 6 years ago

Clean up React propType definitions

Categories

(DevTools :: Memory, defect, P3)

defect

Tracking

(firefox47 wontfix, firefox67 fixed)

RESOLVED FIXED
Firefox 67
Tracking Status
firefox47 --- wontfix
firefox67 --- fixed

People

(Reporter: linclark, Assigned: princewurl510, Mentored)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, Whiteboard: [good first bug][lang=js][polish-backlog])

Attachments

(6 files, 3 obsolete files)

For example, in toolbar.js, the propTypes for censusDisplays and dominatorTreeDisplays should use their respective models as defined in models.js. See Bug 1253650.
Lin, are you taking this follow up bug?
Has STR: --- → irrelevant
Flags: needinfo?(lclark)
Priority: -- → P2
This would probably be a good mentored bug for a new hire or a contributor. I'd be happy to mentor it.
Flags: needinfo?(lclark)
Mentor: lclark
Whiteboard: [good first bug][lang=js][polish-backlog]
Hi Lin, I like to take up this bug. Can you please provide more info, on how to go about it.Thanks.
Flags: needinfo?(lclark)
Thanks for offering to take this on. Someone else had already emailed me yesterday to ask whether they could take the bug. I haven't heard back from them, so I'm going to email them again and see whether they are still planning to do that. I should know in a few days.
Here's more information on how to complete this issue: Each React component needs to say what properties it's going to use. Currently, in toolbar.js the censusDisplays and dominatorTreeDisplays say that they need a displayName property. https://dxr.mozilla.org/mozilla-central/source/devtools/client/memory/components/toolbar.js Instead of having this hardcoded here, :fitzgen would like these to use the censusDisplayModel and dominatorTreeDisplayModel that are defined in models.js. However, these variables aren't exported, so they aren't available to toolbar.js The variables will need to be exported from models.js and imported into toolbar.js. Then they will need to replace the object with the displayName property in the propType definition.
Flags: needinfo?(lclark)
Hi :linclark , Is it still available ? Thanks, Primer
Flags: needinfo?(lclark)
Yes, I believe this is still available.
Flags: needinfo?(lclark)
Hi :lclark , I mailed you by reading above comments what I've understood and also shared the approach I'm going to have. Please check & reply am I on the right track or not. Thanks, Primer
Hi :lclark, Herein I've used already imported modeljs as models , also in model.js censusDisplayModel & dominatorTreeDisplayModel have been exported using export.modules so I think no further change is need in there. If any further changes are required please tell. Thanks, Primer
Flags: needinfo?(lclark)
Attachment #8733488 - Flags: review?(lclark)
Sorry for using :lclark instead of :linclark
Comment on attachment 8733488 [details] [diff] [review] bug1254242_cleanupOfpropTypeDefn.diff Review of attachment 8733488 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch, this is a good start. This isn't currently passing tests. You can check that out by running ./mach test devtools/client/memory/ To fix this, the property name needs to be changed. The way the export is named is a little confusing. This code should actually use models.censusDisplay. That's because in models.js, the export is defined like this `exports.censusDisplay`. The same thing goes for the other one.
Attachment #8733488 - Flags: review?(lclark) → review-
Flags: needinfo?(lclark)
Hi :linclark, Thanks for your input. In this patch I've applied the change suggested by you, also I ran ./mach test devtools/client/memory & it ran successfully. Regards, Primer
Flags: needinfo?(lclark)
Attachment #8733844 - Flags: review?(lclark)
Nice! It's great that the tests are passing. I looked back at what :fitzgen was suggesting in the other patch. It looks like he was suggesting that the model be used directly, instead of constructing the shape here. So basically doing something like this in both cases: > censusDisplays: PropTypes.arrayOf(models.censusDisplay).isRequired, That will make it easier to keep things in sync when folks are adding new properties to the censusDisplayModel. If you make that change and it passes tests, then this patch should be ready :)
Flags: needinfo?(lclark)
Attachment #8733844 - Flags: review?(lclark) → feedback+
Hi :linclark, Thanks for the guiding me further. I've made the changes as per your comment and denominatorTreeDisplay line size exceeds more than 80 character which is against coding standard, just to protect readability I've kept it like this. Again all the tests passed :) Regards, Primer
Flags: needinfo?(lclark)
Attachment #8733957 - Flags: review?(lclark)
Comment on attachment 8733957 [details] [diff] [review] bug1254242_cleanupOfpropTypeDefnv3.diff Review of attachment 8733957 [details] [diff] [review]: ----------------------------------------------------------------- That looks good! Unfortunately I can't get the patch to apply. I'm guessing that there was a change in the file in fx-team that is the problem. It should be a simple fix. Can you try updating your copy of fx-team and recreating the patch? When you do, could you also add r=linclark to the end of the commit message? That's just a little something we do for the commit history.
Attachment #8733957 - Flags: review?(lclark) → feedback+
Hi linclark, I'm sort of stuck in updating fx-team, I did : >hg pull inbound && hg pull fx-team && hg pull aurora && hg pull beta && hg pull esr38 which I found at https://wiki.mozilla.org/Sheriffing/How:To:SheriffingFromUnifiedRepos Since I'm new to hg & workflows followed at Mozilla not sure whether doing right thing or not. It would be quite helpful if you could guide me for things I need to do for your #comment 15 Also after running above command I tried: > hg up fx-team and, than tried checking changes using >hg st -q which didn't show any changes too. Regards, Primer
Flags: needinfo?(lclark)
Whiteboard: [good first bug][lang=js][polish-backlog] → [good first bug][lang=js][polish-backlog][platform-rel-Facebook][platform-rel-ReactJS]
platform-rel: --- → ?
platform-rel: ? → ---
Whiteboard: [good first bug][lang=js][polish-backlog][platform-rel-Facebook][platform-rel-ReactJS] → [good first bug][lang=js][polish-backlog]
[good first bug] whiteboard -> keyword mass change
Keywords: good-first-bug
Hi! Can I take up this issue if not already fixed?
Flags: needinfo?(lclark)
Hi Aman, I've switched jobs and am not on the devtools team anymore, so I'm going to forward this question to someone who is.
Flags: needinfo?(lclark) → needinfo?(pbrosset)
(In reply to Aman Dwivedi from comment #18) > Hi! Can I take up this issue if not already fixed? Sure, the issue is available and not fixed yet. See comment 5 for instructions. Some patches have been created and reviewed already, so I suggest you start from that. Please go through our contribution guide too: https://wiki.mozilla.org/DevTools/Hacking I'll assign the bug to you as soon as you've started working on this. Thanks for your help!
Flags: needinfo?(pbrosset)
Hello, I'm interested in taking this on as my first bug if Aman from Comment 18 doesn't end up picking it up.
Flags: needinfo?(pbrosset)
Attached patch BugFix1254242.patch (obsolete) — Splinter Review
Hi! Sorry for delay. This patch should resolve the issue. Also, I ran './mach test devtools/client/memory' & it ran successfully. Comments?
Attachment #8829093 - Flags: review?(pbrosset)
Mentor: lclark
Flags: needinfo?(pbrosset)
Assigned to Aman. I'll forward the review request to someone else because I am not familiar with this part of the code or propTypes.
Assignee: nobody → dwivedi.aman96
Status: NEW → ASSIGNED
Comment on attachment 8829093 [details] [diff] [review] BugFix1254242.patch Review of attachment 8829093 [details] [diff] [review]: ----------------------------------------------------------------- Hi Greg, are you able to review this propType change? I'm not sure what's supposed to be done here. I was just helping move the bug forward.
Attachment #8829093 - Flags: review?(pbrosset) → review?(gtatum)
Comment on attachment 8829093 [details] [diff] [review] BugFix1254242.patch Review of attachment 8829093 [details] [diff] [review]: ----------------------------------------------------------------- Almost there! I think we should be able to remove all uses of the PropTypes.shape in this file with the models. I'll r+ it once they are all changed over. Thanks for doing this! ::: devtools/client/memory/components/toolbar.js @@ +13,4 @@ > displayName: "Toolbar", > > propTypes: { > + censusDisplays: PropTypes.arrayOf(models.censusDisplay).isRequired, Looks great. @@ +17,1 @@ > censusDisplay: PropTypes.shape({ We should be able to use the models here anywhere there is a PropTypes.shape. @@ +33,1 @@ > labelDisplay: PropTypes.shape({ models.labelDisplay
Attachment #8829093 - Flags: review?(gtatum)
Please review this patch. Thanks!
Attachment #8829093 - Attachment is obsolete: true
Attachment #8830653 - Flags: review?(gtatum)
Hello! How to get this patch reviewed and get it landed? Seems Greg is not available.
I've been focusing on another project and this fell through the cracks. I'll take a look at it today.
Comment on attachment 8830653 [details] [diff] [review] BugFix1254242.patch Review of attachment 8830653 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for moving this forward! I'm getting some errors with these changes: Warning: Failed prop type: Required prop `censusDisplay.isRequired` was not specified in `Toolbar`. in Toolbar (created by MemoryApp) in MemoryApp (created by Connect(MemoryApp)) in Connect(MemoryApp) (created by bound createElement) in bound createElement in Provider react-dev.js:22807:9 Warning: Failed prop type: Required prop `labelDisplay.isRequired` was not specified in `Toolbar`. in Toolbar (created by MemoryApp) in MemoryApp (created by Connect(MemoryApp)) in Connect(MemoryApp) (created by bound createElement) in bound createElement in Provider I believe these errors will only show up if you have the following in your mozconfig file: ac_add_options --enable-debug-js-modules I'm clearing the review for now. Are you able to reproduce this?
Attachment #8830653 - Flags: review?(gtatum)
Sorry Greg, I'm unable to reproduce this. Which 'mozconfig' file do I need to change in order to do this. I'm unable to find the relevant 'mozconfig' file.
Flags: needinfo?(gtatum)
You should be able to create a text file named "mozconfig" in your source directory, and then add ac_add_options --enable-debug-js-modules to it to enable the debug information. https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options
Flags: needinfo?(gtatum)
Hey Greg! I would like to take this up if Aman is no longer working on it.
Flags: needinfo?(gtatum)
Hey Aman, are you still working on this? Thanks!
Flags: needinfo?(dwivedi.aman96)
(In reply to Julien Wajsberg [:julienw] from comment #33) > Hey Aman, are you still working on this? Thanks! Hey Julien, I was caught in some other work since a long time. I'm not working on this. Someone else can take it up.
Flags: needinfo?(dwivedi.aman96)
Aman, thanks for your quick answer! Thanks for your help! Tushar, then I assign the bug to you, thanks! Please ask on this bug if you need anything. We're also on IRC (irc.mozilla.org, channel #devtools) and Slack (go to https://devtools-html-slack.herokuapp.com/ first and register, we're on channel #perf). I believe the code hadn't changed much for some time so the existing patches should still apply and I think you can start from there.
Assignee: dwivedi.aman96 → tushararora.cs
Flags: needinfo?(gtatum)
Comment on attachment 8944151 [details] Bug 1254242 Clean up React propType definitions Hey Greg! Could you please review the commit? Thanks!
Attachment #8944151 - Flags: review?(gtatum)
Product: Firefox → DevTools
What is the status of this bug? I would like to work on it if there is more that needs to be done. Thanks.
Attachment #8944151 - Flags: review?(gtatum)
Sorry, I removed myself from this review, as I've been focusing on other tools, and haven't been able to help with this work.

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Assignee: tushararora.cs → nobody
Status: ASSIGNED → NEW

(In reply to Patrick Brosset <:pbro> from comment #40)

This bug has not been updated in the last 3 months. Resetting the assignee field.
Please, feel free to pick it up again and add a comment outlining your plans for it if you do still intend to work on it.
This is just trying to clean our backlog of bugs and make bugs available for people.

Hello Patrick would love to work on this bug and I am an Outreachy applicant. Any suggestions on where to start?

Are the Artificial builds sufficient to work on this bug

Assignee: nobody → princewurl510
Mentor: nchevobbe
Priority: P2 → P3
Attachment #9047594 - Attachment is obsolete: true
Attachment #9048303 - Attachment description: Address revision issues: updated censusDisplay and labelDisplay → Bug 1254242 - Clean up React propType definitions in devtools/client/memory/components/Toolbar.js. r=nchevobbe.
Pushed by nchevobbe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e31662ecc9d1 Clean up React propType definitions in devtools/client/memory/components/Toolbar.js. r=nchevobbe.
Attachment #9047920 - Attachment is obsolete: true
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: