Improve AppProvider and UserProvider rendering perf #5215
Conversation
These providers would vend stale state into their context after props changing, which would require another unnecessary full re-render to get updated context values via `useEffect` hooks. Instead, now state is updated during render so as to immediately restart rendering before stale state is rendered down the context, which is more efficient. For more information on this approach, please see the [Adjusting some state when a props changes](https://beta.reactjs.org/learn/you-might-not-need-an-effect#adjusting-some-state-when-a-prop-changes) section of React beta docs on the "You Might Not Need an Effect" page.
|
Realm welcomes all contributions! The only requirement we have is that, like many other projects, we need to have a Contributor License Agreement (CLA) in place before we can accept any external code. Our own CLA is a modified version of the Apache Software Foundation’s CLA. Our records show that CLA has not been signed by @appden. Please submit your CLA electronically using our Google form so we can accept your submissions. After signing the CLA you can recheck this PR with a |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Hey Scott - lovely to see a PR from you again ;-) Thanks! |
|
Thanks for your PR! @takameyer which is normally the one reviewing PRs for our React package is OoO until the beginning of next week. I hope we'll get this reviewed shortly thereafter. |
| }, [configVersion, setApp]); | ||
| } | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
I think this useEffect could also go away and the logic moved under line 51. Thoughts?
There was a problem hiding this comment.
Thanks for taking a look at this @takameyer! Your asking this made me realize this technically should be useLayoutEffect, so I'll push that update. This is a nuanced one, especially with React's concurrent mode semantics coming to React Native. Refs are typically updated by React right before running layout effects, not during render since a render pass can be thrown away. While it's fine (ish) to update a ref you "own" during render (though that can lead to trouble too), refs you don't own should be updated as side effects because a render can be thrown away with concurrent mode. Also, another benefit here is that if appRef itself changes for some reason, then it'll get updated accordingly. 😀
There was a problem hiding this comment.
Now that you mention that, I will have to give the generated RealmProvider a look as well. This could most likely also benefit from useLayoutEffect when binding the realmRef.
There was a problem hiding this comment.
Hi @takameyer, checking to see if this can be merged. I see a bunch of CI jobs are failing in the same way that appears to be unrelated...
An unexpected error occurred: Input required and not supplied: projectId
Refs are typically updated by React before running layout effects, so this better matches those semantics.
What, How & Why?
These providers would vend stale state into their context after props changing, which would require another unnecessary full re-render to get updated context values via
useEffecthooks. Instead, now state is updated during render so as to immediately restart rendering before stale state is rendered down the context, which is more efficient.For more information on this approach, please see the Adjusting some state when a props changes section of React beta docs on the "You Might Not Need an Effect" page.