[Experimental] API for reading context from within any render phase function #13139
Conversation
|
React: size: 🔺+2.4%, gzip: 🔺+1.9% ReactDOM: size: 🔺+0.7%, gzip: 🔺+0.7% Details of bundled changes.Comparing: 5776fa3...47d4520 react
react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
Generated by 🚫 dangerJS |
|
This is a larger file increase than I would've expected. Do we understand why? |
|
Exciting! Do you know if this will break the |
|
No, that API is not being supported going forward. Use a portal please :) (Do portals not work for you?) |
| @@ -135,6 +135,7 @@ import { | |||
| commitAttachRef, | |||
| commitDetachRef, | |||
| } from './ReactFiberCommitWork'; | |||
| import * as dispatcher from './ReactFiberDispatcher'; | |||
There was a problem hiding this comment.
We should avoid exporting everything. Make it explicit what you want this object to contain. Too easy to accidentally bring in too much. For example, I think the resulting object that rollup produces for this is not what you expect. (Let me know if you see this pattern elsewhere so we can fix it.)
There was a problem hiding this comment.
I figured it was ok in this case because I need to use an object for dynamic dispatch. But I agree explicit is better.
|
I figured that, but that would still make it a breaking change for libraries like react-redux I suppose (which is still using the legacy context API reduxjs/react-redux#898). Just wanted to bring it up though :) And yea, portals don't work for reparenting #13044 - But that was risky all along. |
|
Thought of a test case: reading context inside |
| } | ||
| } | ||
|
|
||
| export function checkForPendingContext( |
There was a problem hiding this comment.
This function is way too slow to iterate through these every time we rerender even if we're about to just rerender anyway. We'll need a different approach.
This should already have been known by the context propagation so we just need some way of storing that.
There was a problem hiding this comment.
What about the part where we continue the propagation where we left off?
There was a problem hiding this comment.
We could do that part only if any of the contexts matched/changed. The point is to optimize for the case when zero context changed.
There was a problem hiding this comment.
A different approach would be to keep track of the currently rendering fiber. Which we already do with current owner, so maybe we can find some overlap there. We're also going to need that for other things anyway.
We could set the current fiber in the beginning of beginWork so that it is properly set for all code branches since essentially anything can call this. So we save some code there.
Then right before render we reset firstContextReader to null on that fiber. It'll be local anyway since it is used by that function.
Then in the context reader function we check if firstContextReader is null. If it is, then reset lastContextReader (that one is still global but you can get rid of firstContextReader) to the new entry. If it is not, then use append it to the lastContextReader.
Note how this minimizes the number of operations we have to do when no context is used.
| function getContextChangedBits(context: ReactContext<any>): number { | ||
| return isPrimaryRenderer ? context._changedBits : context._changedBits2; | ||
| export function prepareToReadContext(): void { | ||
| nextFirstReader = nextLastReader = null; |
There was a problem hiding this comment.
You do this in finishReadingContext also. Only it do it once per loop iteration. Either before the loop and in finishReadingContext or in prepareToReadContext. If you keep prepareToReadContext, we should inline all callsites. I don't trust tooling to make this fast.
| popProvider(providerFiber: Fiber): void, | ||
| getContextCurrentValue(context: ReactContext<any>): any, | ||
| getContextChangedBits(context: ReactContext<any>): number, | ||
| export type ContextReader<T> = { |
| @@ -124,6 +125,9 @@ export type Fiber = {| | |||
| // The state used to create the output | |||
| memoizedState: any, | |||
|
|
|||
| // A linked-list of contexts that this fiber depends on | |||
| firstContextReader: ContextReader<mixed> | null, | |||
|
@sebmarkbage Updated |
| changedBits, | ||
| renderExpirationTime, | ||
| ); | ||
| if ((changedBits & dependency.observedBits) !== 0) { |
There was a problem hiding this comment.
@sebmarkbage As you pointed out, we already did this check while scanning from the provider. But where to store that data? On the update queue, perhaps? I'd rather not put it in the expiration time, if we can avoid it, since that field is already pretty confusing and overloaded.
|
Went with this approach for now: Track whether any contexts have changed, on the stack. If this flag is false, we can skip checking any context dependencies. Since it's unusual that more than one context updates within the same batch, this should save us a lot of unnecessary checks. (EDIT: It's more about optimizing for the case where no contexts have changed at all, especially high priority updates.) |
Is it? If people don't listen to our recommendations and write Since updates at the very top don't happen that often for most apps, this issue isn't very noticeable, and I'd expect that when popular libraries start using context more, it can become a problem. Note I haven't read your code so I might have misunderstood your comment. |
|
@gaearon I don't think there's much we can do to optimize for that case, anyway. This is more about optimizing for the case where no contexts have changed at all. Especially during a high priority update. |
acdlite
force-pushed
the
unifycontext
branch
2 times, most recently
from
d4cd317
to
9c606b7
Compare
|
Ok, new approach for bailouts after discussing with @sebmarkbage: Currently, the The latest commit adds a second field called The biggest advantage is it requires fewer checks to bailout on already finished work. For most types of work, if the |
|
Is this mainly for theming or as a full alternative to For example, in Formik's |
|
What do you mean by theming? I don't think this will help your use case because |
Oh, I missed this. Hmm. If you're okay with defining it in render you can already do today with This method just provides some convenience, and is intended for library use cases. We'll share more later. |
by theming, i mean a top-level context without a provider, mostly static values i guess.
I could just . |
Hmm no, your |
|
Doh! I haven't had my coffee yet this am. 😜Makes sense. |
|
Any ETA on this being released? I suspect that React-Redux v6 will need to make use of this, and I'm hoping to do some hacking on it next week at React Rally. (Yeah, I know I can do a custom build of React locally, but would also be good to know when it might be out officially.) |
|
It is likely that whatever is exposed is a different API than this one. |
|
What differences would you expect to see from what's here? |
|
@markerikson Can you explain a bit more about why you need it? Why isn't the render prop API sufficient? |
|
@sebmarkbage : React-Redux's We plan to switch to new context in v6, and my original proof-of-concept experiment in reduxjs/react-redux#898 puts the store state into context rather than the whole store, with the hope that that is more async-compatible (and possibly even more performant). While trying to update our 5.x branch to not warn in StrictMode, we've come up with changes that seem to mostly behave, and rely on doing work in Meanwhile, the current advice for accessing new context values in lifecycle methods is "create an intermediate component" ( https://reactjs.org/docs/context.html#accessing-context-in-lifecycle-methods ), but that means adding a second level of wrapping, which I'd like to avoid if possible. Ultimately, what it boils down to is that if we want to keep a single wrapper component instead of two levels of wrapper components, we almost definitely need to read the store state from context in render-phase methods in that one wrapper component, rather than doing all the work in the (Please feel free to correct me if I'm mistaken in some of my assumptions somewhere.) |
|
To manage refs you can use forwardRef to forward any ref to the inner component to avoid wrappers for the HoC itself. Note that this API doesn't let you access context in life-cycles. So you need some way to stash it if you expect to use it life-cycles which requires you to resort to hacks anyway. The second level of wrapping helps avoid the need to do that since it's accessible from props. It sounds more like you would want this API: #13336 (comment) (I will note that avoiding wrappers is in itself an anti-pattern. It's not really all that much slower to have a wrapper than the extra objects required by this API. I understand this is annoying in the devtools but we should just fix that there with blackboxing.) |
|
I don't think Mark is asking about ref-management. My take on his comment is that he's asking how to avoid creating two wrapper components/fibers for every connected user component, which the latter part of Seb's response targets. If the remaining concern then is DevTools overhead– we should work together to eliminate that. Maybe this means filtering out types of fibers, maybe it means a more custom solution like this one. |
|
Yeah, what Brian said. There's a few reasons for this:
If we were to go to two wrappers and use of a That starts to get awfully silly. I'm certainly open to suggestions on the best way to approach implementing this. |
|
Sounds like there are two concerns: (1) performance and (2) DevTools UX Sounds like (1) may not really be a problem (or at least that we should try it first and see). As for (2), we have an open DevTools issue that is related. If we provided a filtering option for HOCs, for example, that would prune your example tree above to: We also plan to add the option to filter certain types of fibers (e.g. context) which could further trim it down to: Not sure what the exact UI/UX for this will be yet, but it's something I think we can solve fairly easily on the DevTools side. |
|
Yeah, I've been trying to set up some perf benchmarks so that I can easily test multiple versions of React-Redux against each other, as a precursor to actually sitting down and working on v6. Made some progress - I've got a small Puppeteer harness that can run a specific benchmark and report FPS results (using I've sure you've got plenty of plans for ReactRally, but maybe I can steal a few minutes of your time while we're there? I'm hoping to hack on v6 Saturday and Sunday, so any advice I can get would be appreciated :) |
|
We'll see how things go! I'll be there for less than two days, so I might not have a ton of free time, but we can probably chat for a little bit at least. |
|
Just to confirm - it's not avaliable anymore? |
Context.unstable_readcan be called anywhere within the render phase. That includes the render method,getDerivedStateFromProps, constructors, functional components, and context consumer render props.