[core] List the requirement of each hooks #2319
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduction
The goal of this PR is to give a clearer view of which hooks is using state / methods / events from other hooks to anticipate the effects of synchronous state updates.
In the example below, I will always use the following hooks in the same order :
Current behavior
The hook order matter only for usages that have the same initialization / update timing.
Methods
State
Impact of the state management rework
One of the goal of #2231 is to reduce the
setGridState
cascade inuseEffect
that causes chain of re-rendersWe have to make some tests to determine the best approach, but the goal is that a change in the props or the call of a DOM event updates synchronously as much of the state as possible.
We have two main approaches right now : apply the derived state in the render (like explained here) or apply the derived state with synchronous events (like we are already doing a lot).
In either way, this should have pretty much the same impact on this PR goal.
The following scenarios should be impacted
Use a state in a now synchronous event
For events, we may bypass the issue by registering the calls and running them all after all hooks are executed.
But it would add some complexity.
Without it, the following example would crash on the 1st render (
getB
would not be defined) and would show outdatedpropForFeatureB
on future renders.Use a selector
If we go with the approach n°1 in #2293 , then the state would not be initialized in the 1st run of
useFeatureA
If we go with the approach n°2, it would be initialized but for future run it would be outdated because
useFeatureB
would update it afteruseFeatureA
runs.Solutions
Try to avoid circular feature dependencies (
useFeatureA
needs state fromuseFeatureB
which needs method fromuseFeatureC
which needs state fromuseFeatureA
).If their is a circular feature dependency, then one dependency should only be in a DOM event callback to break the chain (for instance if
useFeatureA
needs auseFeatureB
state in the render anduseFeatureB
needs auseFeatureA
method but in aGridEvents.rowClick
event, then it should not be an issue as long as thehandleRowClick
callback does not use any selectors executed in the render.The difficult scenarios occurs when
useFeatureA
needs something fromuseFeatureB
which is used in a method exposed byuseFeatureA
anduseFeatureB
needs something fromuseFeatureA
in the render. Because it is way harder to track if the method exposed byuseFeatureA
is ever called synchronously. In that scenario, the best thing is to try to executeuseFeatureB
just afteruseFeatureA
so that any hook afteruseFeatureB
is also afteruseFeatureA
But if we can have almost no circular feature dependency and therefore apply a perfect hook execution order. Then it would be a lot easier.