-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[No QA] Implement Why Did You Render #4260
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good just a couple comments. Nice tool.
wdyr.js
Outdated
include: [ | ||
/^Avatar/, | ||
/^ReportActionItem/, | ||
/^ReportActionItemSingle/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make it so all things are included by default? And maybe comment out this include/exclude section as optional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is A LOT of noise when tracking every component and I'm not sure that opening my console to 100+ notifications about unrelated components would be that useful.
I think that tracking relevant components might be better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand, but also the feature is opt-in. So maybe the default would be to show everything unless otherwise specified. It's not difficult to filter for the exact component by filtering in the JS console. Mostly bringing this up because I have used this tool before and don't usually futz with the configs too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe as a compromise we can just mention how to show everything in a commen? :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original GH did specify:
Implement WDYR and only enable it on certain "problem components" like ReportActionItem, Avatar etc.
But I agree having a comment here about how to enable it for everything would be nice to have.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, what can I say, my past and future selves don't always get along ! 😂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough! I made changes to display everything by default and left the comment on how to include/exclude certain components only.
also this can probably be No QA instead of Internal QA. |
Updated! |
wdyr.js
Outdated
include: [ | ||
/^Avatar/, | ||
/^ReportActionItem/, | ||
/^ReportActionItemSingle/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The original GH did specify:
Implement WDYR and only enable it on certain "problem components" like ReportActionItem, Avatar etc.
But I agree having a comment here about how to enable it for everything would be nice to have.
Updated! |
const whyDidYouRender = require('@welldone-software/why-did-you-render'); | ||
whyDidYouRender(React, { | ||
// Tracks all components by default | ||
trackAllPureComponents: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be false
(the default)? PureComponent seems like the least likely to be re-rendering for bad reasons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After doing more research and seeing this comment from one of the creators, it seems that tracking pure components by default is a better idea, as they might control big render trees.
So I will keep that as the default, but I also included a way to enable tracking in all components, as well as examples to enable tracking by component displayName
.
Screen
must be excluded when tracking all components to avoid the following error that crashes the app:
Uncaught Error: A navigator can only contain 'Screen', 'Group' or 'React.Fragment' as its direct children (found 'WDYRFunctionalComponent' for the screen 'Home'). To render this component in the navigator, pass it in the 'component' prop to 'Screen'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok that works. Weird about the Screen
component. Must have used this last before we added react navigation.
455b30a
to
901acd2
Compare
Updated! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging in version: 1.0.82-8🚀
|
🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀
|
cc @marcaaron @chiragsalian
Details
Implements Why Did You Render to help track and avoid unnecessary re-renders. Once enabled, it will track re-renders of
Avatar
,ReportActionItem
andReportActionItemSingle
. More components can be added inwdyr.js
.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/165181
Tests
USE_WDYR=true
in your.env
file.npm run web
QA Steps
Internal QA
Tested On
Screenshots
Web
Mobile Web
Desktop
iOS
Android