1254242 - Clean up React propType definitions

Status: RESOLVED FIXED

(DevTools :: Memory, defect, P3)

Description

[Lin Clark [:linclark]]

For example, in toolbar.js, the propTypes for censusDisplays and dominatorTreeDisplays should use their respective models as defined in models.js. See Bug 1253650.

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Lin, are you taking this follow up bug?

Comment 2

[Lin Clark [:linclark]]

This would probably be a good mentored bug for a new hire or a contributor. I'd be happy to mentor it.

Comment 3

[simplyblue24]

Hi Lin, I like to take up this bug. Can you please provide more info, on how to go about it.Thanks.

Comment 4

[Lin Clark [:linclark]]

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.

Comment 5

[Lin Clark [:linclark]]

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.

Comment 6

[:primer]

Hi :linclark ,

Is it still available ?

Thanks,
Primer

Comment 7

[Lin Clark [:linclark]]

Yes, I believe this is still available.

Comment 8

[:primer]

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

[:primer]

Attachment: bug1254242_cleanupOfpropTypeDefn.diff

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

Comment 10

[:primer]

Sorry for using :lclark instead of :linclark

Comment 11

[Lin Clark [: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.

Comment 12

[:primer]

Attachment: bug1254242_cleanupOfpropTypeDefnv2.diff

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

Comment 13

[Lin Clark [:linclark]]

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 :)

Comment 14

[:primer]

Attachment: bug1254242_cleanupOfpropTypeDefnv3.diff

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

Comment 15

[Lin Clark [:linclark]]

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.

Comment 16

[:primer]

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

Comment 17

[Patrick Brosset <:pbro>]

[good first bug] whiteboard -> keyword mass change

Comment 18

[Aman Dwivedi]

Hi! Can I take up this issue if not already fixed?

Comment 19

[Lin Clark [:linclark]]

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.

Comment 20

[Patrick Brosset <:pbro>]

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

Comment 21

[frankanthonyserrano]

Hello,

I'm interested in taking this on as my first bug if Aman from Comment 18 doesn't end up picking it up.

Comment 22

[Aman Dwivedi]

Attachment: BugFix1254242.patch

Hi! Sorry for delay. This patch should resolve the issue. Also, I ran 
'./mach test devtools/client/memory' & it ran successfully. Comments?

Comment 23

[Patrick Brosset <:pbro>]

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.

Comment 24

[Patrick Brosset <:pbro>]

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.

Comment 25

[Greg Tatum [:gregtatum]]

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

Comment 26

[Aman Dwivedi]

Attachment: BugFix1254242.patch

Please review this patch. Thanks!

Comment 27

[Aman Dwivedi]

Hello! How to get this patch reviewed and get it landed? Seems Greg is not available.

Comment 28

[Greg Tatum [:gregtatum]]

I've been focusing on another project and this fell through the cracks. I'll take a look at it today.

Comment 29

[Greg Tatum [:gregtatum]]

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?

Comment 30

[Aman Dwivedi]

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.

Comment 31

[Greg Tatum [:gregtatum]]

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

Comment 32

[Tushar Arora [:tushararora]]

Hey Greg! I would like to take this up if Aman is no longer working on it.

Comment 33

[Julien Wajsberg [:julienw]]

Hey Aman, are you still working on this? Thanks!

Comment 34

[Aman Dwivedi]

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

Comment 35

[Julien Wajsberg [:julienw]]

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.

Comment 36

[Tushar Arora [:tushararora]]

Attachment: Bug 1254242 Clean up React propType definitions

Review commit: https://reviewboard.mozilla.org/r/214454/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/214454/

Comment 37

[Tushar Arora [:tushararora]]

Comment on attachment 8944151 [details]
Bug 1254242 Clean up React propType definitions

Hey Greg! Could you please review the commit? Thanks!

Comment 38

[arika.arnzen]

What is the status of this bug? I would like to work on it if there is more that needs to be done. Thanks.

Comment 39

[Greg Tatum [:gregtatum]]

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.

Comment 40

[Patrick Brosset <:pbro>]

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.

Comment 41

[Adrian Anderson]

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

Comment 42

[Adrian Anderson]

Are the Artificial builds sufficient to work on this bug

Comment 43

[Adrian Anderson]

Attachment: Bug 1254242 - Clean up React propType definitions r=Julien Wajsberg [:julienw]

Comment 44

[Adrian Anderson]

Attachment: Bug 1254242 - Clean up React propType definitions r=nchevobbe

Comment 45

[Adrian Anderson]

Attachment: Bug 1254242 - Clean up React propType definitions in devtools/client/memory/components/Toolbar.js. r=nchevobbe.

Comment 46

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e31662ecc9d1
Clean up React propType definitions in devtools/client/memory/components/Toolbar.js. r=nchevobbe.

Comment 47

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e31662ecc9d1