Decouple update queue from Fiber type - #12600
Conversation
6fdc713 to
9ae2db3
Compare
There was a problem hiding this comment.
This is because we weren't counting componentDidCatch as a lifecycle previously
|
ReactDOM: size: 🔺+0.4%, gzip: -0.2% Details of bundled changes.Comparing: 999b656...e4ce363 react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
|
Need to add “update DevTools” to todos. I think it might rely on |
I don't think so? I don't see any references to |
|
Note: the bundle size changes are mostly because I haven't deleted the old update queue yet :D |
e325697 to
a99b4ca
Compare
acdlite
changed the title
|
@sebmarkbage Ready for review |
There was a problem hiding this comment.
I don't know why this wasn't already 0. There are no host effects or lifecycles in the second flush. New snapshot seems correct.
|
Needless to say, this is a risky change. I'll do the next www sync and fix any bugs that may come up. |
What does [redacted] mean? |
There was a problem hiding this comment.
What is this change? You didn't replicate the other assignment so something is new.
There was a problem hiding this comment.
There was a problem hiding this comment.
DRY for DRYness sake? Makes it harder to read this inline IMO. It also encourages reading the getDerivedStateFromProps property in multiple places if they're also needed at the callsite. Only abstract if it is chunk of work. (On the flip side, this PR successfully unabstract memoizeProps and memoizeState in places.)
There was a problem hiding this comment.
There was a reason we had to apply these effects in the complete phase. Why are we able to move them to the begin phase now?
There was a problem hiding this comment.
I don't think there was a reason. It happened to be a convenient place to do it. Now we do it while processing the update queue.
There was a problem hiding this comment.
What are theses? They're not described in terms of side-effects. Either this needs to change, or the whole thing renamed.
There was a problem hiding this comment.
I really had the term "Shared". This has been sneaking into RN too.
It doesn't describe anything about the concept other than DRYness which indicates that maybe you should repeat yourself. If it is real concept, name it.
There was a problem hiding this comment.
Stop denying me muh mixins!
There was a problem hiding this comment.
This is clearly not going to be how we describe "redacted" so seems like a too early abstraction. Even just assuming that it is a single argument is a big assumption. Just use Fiber until you have something else.
There was a problem hiding this comment.
You are forking into a first class concept with typeOfUpdateQueue then you're picking separate code paths again in commitEffect. The only thing gain by doing that instead of specializing each function is that you can reuse the code above here. Just break this out into a reusable function and instead specialize each code path without making typeOfUpdateQueue a first class thing.
|
TODO after chatting to @sebmarkbage:
|
995a437 to
73d4d48
Compare
The update queue is in need of a refactor. Recent bugfixes (#12528) have
exposed some flaws in how it's modeled. Upcoming features like Suspense
and [redacted] also rely on the update queue in ways that weren't
anticipated in the original design.
Major changes:
- Instead of boolean flags for `isReplace` and `isForceUpdate`, updates
have a `tag` field (like Fiber). This lowers the cost for adding new
types of updates.
- Render phase updates are special cased. Updates scheduled during
the render phase are dropped if the work-in-progress does not commit.
This is used for `getDerivedStateFrom{Props,Catch}`.
- `callbackList` has been replaced with a generic effect list. Aside
from callbacks, this is also used for `componentDidCatch`.
73d4d48 to
8830fd2
Compare
I tried to avoid this at first, since we avoid it everywhere else in the Fiber codebase, but since updates are not in a hot path, the trade off with file size seems worth it.
* Decouple update queue from Fiber type The update queue is in need of a refactor. Recent bugfixes (react#12528) have exposed some flaws in how it's modeled. Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design. Major changes: - Instead of boolean flags for `isReplace` and `isForceUpdate`, updates have a `tag` field (like Fiber). This lowers the cost for adding new types of updates. - Render phase updates are special cased. Updates scheduled during the render phase are dropped if the work-in-progress does not commit. This is used for `getDerivedStateFrom{Props,Catch}`. - `callbackList` has been replaced with a generic effect list. Aside from callbacks, this is also used for `componentDidCatch`. * Remove first class UpdateQueue types and use closures instead I tried to avoid this at first, since we avoid it everywhere else in the Fiber codebase, but since updates are not in a hot path, the trade off with file size seems worth it. * Store captured errors on a separate part of the update queue This way they can be reused independently of updates like getDerivedStateFromProps. This will be important for resuming. * Revert back to storing hasForceUpdate on the update queue Instead of using the effect tag. Ideally, this would be part of the return type of processUpdateQueue. * Rename UpdateQueue effect type back to Callback I don't love this name either, but it's less confusing than UpdateQueue I suppose. Conceptually, this is usually a callback: setState callbacks, componentDidCatch. The only case that feels a bit weird is Timeouts, which use this effect to attach a promise listener. I guess that kinda fits, too. * Call getDerivedStateFromProps every render, even if props did not change Rather than enqueue a new setState updater for every props change, we can skip the update queue entirely and merge the result into state at the end. This makes more sense, since "receiving props" is not an event that should be observed. It's still a bit weird, since eventually we do persist the derived state (in other words, it accumulates). * Store captured effects on separate list from "own" effects (callbacks) For resuming, we need the ability to discard the "own" effects while reusing the captured effects. * Optimize for class components Change `process` and `callback` to match the expected payload types for class components. I had intended for the update queue to be reusable for both class components and a future React API, but we'll likely have to fork anyway. * Only double-invoke render phase lifecycles functions in DEV * Use global state to track currently processing queue in DEV
…ed (react#12802) Fixes an oversight from react#12600. getDerivedStateFromProps should fire if either props *or* state have changed, but not if *neither* have changed. This prevents a parent from re-rendering if a deep child receives an update.
8 participants
The update queue is in need of a refactor. Recent bugfixes (#12528) have exposed some flaws in how it's modeled. Upcoming features like Suspense and [redacted] also rely on the update queue in ways that weren't anticipated in the original design.
Major changes:
isReplaceandisForceUpdate, updates have atagfield (like Fiber). This lowers the cost for adding new types of updates.getDerivedStateFrom{Props,Catch}.callbackListhas been replaced with a generic effect list. Aside from callbacks, this is also used forcomponentDidCatch.TODO:
Update React DevTools dependency onI reverted this changehasForceUpdate(in a separate repo)