Skip to content
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

Merged
merged 6 commits into from
Aug 4, 2021
Merged

[No QA] Implement Why Did You Render #4260

merged 6 commits into from
Aug 4, 2021

Conversation

luacmartins
Copy link
Contributor

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 and ReportActionItemSingle. More components can be added in wdyr.js.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/165181

Tests

  1. Set USE_WDYR=true in your .env file.
  2. Run npm run web
  3. Open the browser console and see notifications from Why Did You Render.
  4. Profit!

QA Steps

Internal QA

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Screen Shot 2021-07-27 at 4 02 31 PM

Mobile Web

Desktop

iOS

Android

@luacmartins luacmartins requested a review from a team as a code owner July 27, 2021 22:25
@luacmartins luacmartins self-assigned this Jul 27, 2021
@MelvinBot MelvinBot requested review from deetergp and removed request for a team July 27, 2021 22:25
Copy link
Contributor

@marcaaron marcaaron left a 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 Show resolved Hide resolved
wdyr.js Outdated Show resolved Hide resolved
wdyr.js Outdated
include: [
/^Avatar/,
/^ReportActionItem/,
/^ReportActionItemSingle/,
Copy link
Contributor

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?

Copy link
Contributor Author

@luacmartins luacmartins Jul 28, 2021

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.

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor

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.

Copy link
Contributor

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

Copy link
Contributor Author

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.

@marcaaron
Copy link
Contributor

also this can probably be No QA instead of Internal QA.

@marcaaron marcaaron changed the title [InternalQA] Implement Why Did You Render Implement Why Did You Render Jul 27, 2021
@marcaaron marcaaron changed the title Implement Why Did You Render [No QA] Implement Why Did You Render Jul 27, 2021
@luacmartins
Copy link
Contributor Author

Updated!

wdyr.js Outdated
include: [
/^Avatar/,
/^ReportActionItem/,
/^ReportActionItemSingle/,
Copy link
Contributor

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.

@luacmartins
Copy link
Contributor Author

Updated!

const whyDidYouRender = require('@welldone-software/why-did-you-render');
whyDidYouRender(React, {
// Tracks all components by default
trackAllPureComponents: true,
Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

@luacmartins
Copy link
Contributor Author

Updated!

@marcaaron marcaaron requested a review from deetergp August 3, 2021 17:13
@deetergp deetergp merged commit 2591e8c into main Aug 4, 2021
@deetergp deetergp deleted the cmartins-wdyr branch August 4, 2021 17:35
@OSBotify
Copy link
Contributor

OSBotify commented Aug 4, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants