Clean up React propType definitions

RESOLVED FIXED in Firefox 67

Status

defect
P3
normal
RESOLVED FIXED
3 years ago
3 months ago

People

(Reporter: linclark, Assigned: princewurl510, Mentored)

Tracking

(Blocks 1 bug, {good-first-bug})

Trunk
Firefox 67
Dependency tree / graph

Firefox Tracking Flags

(firefox47 affected, firefox67 fixed)

Details

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

Attachments

(6 attachments, 3 obsolete attachments)

Reporter

Description

3 years ago
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
Reporter

Comment 2

3 years ago
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]

Comment 3

3 years ago
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)
Reporter

Comment 4

3 years ago
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.
Reporter

Comment 5

3 years ago
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)

Comment 6

3 years ago
Hi :linclark ,

Is it still available ?

Thanks,
Primer
Flags: needinfo?(lclark)
Reporter

Comment 7

3 years ago
Yes, I believe this is still available.
Flags: needinfo?(lclark)

Comment 8

3 years ago
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

Comment 9

3 years ago
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)

Comment 10

3 years ago
Sorry for using :lclark instead of :linclark
Reporter

Comment 11

3 years ago
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-
Reporter

Updated

3 years ago
Flags: needinfo?(lclark)

Comment 12

3 years ago
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)
Reporter

Comment 13

3 years ago
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)
Reporter

Updated

3 years ago
Attachment #8733844 - Flags: review?(lclark) → feedback+

Comment 14

3 years ago
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)
Reporter

Comment 15

3 years ago
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+

Comment 16

3 years ago
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

Comment 18

2 years ago
Hi! Can I take up this issue if not already fixed?
Flags: needinfo?(lclark)
Reporter

Comment 19

2 years ago
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)

Comment 22

2 years ago
Posted 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)

Comment 26

2 years ago
Please review this patch. Thanks!
Attachment #8829093 - Attachment is obsolete: true
Attachment #8830653 - Flags: review?(gtatum)

Comment 27

2 years ago
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)

Comment 30

2 years ago
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)

Comment 34

a year ago
(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)

Updated

a year ago
Product: Firefox → DevTools

Comment 38

9 months ago
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
Assignee

Comment 41

3 months ago

(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?

Assignee

Comment 42

3 months ago

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.

Comment 46

3 months ago
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

Comment 47

3 months ago
bugherder
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67
You need to log in before you can comment on or make changes to this bug.