Move RAF check to first render() #11117
Conversation
|
Deploy preview ready! Built with commit a756039 |
| @@ -731,6 +749,7 @@ var ReactDOMFiber = { | |||
| container: DOMContainer, | |||
| callback: ?Function, | |||
| ) { | |||
| checkForRAF(); | |||
| return renderSubtreeIntoContainer( | |||
There was a problem hiding this comment.
Can we move into renderSubtreeIntoContainer since it covers more code paths?
|
Can you verify if it fixes the setup in facebook/create-react-app#3199 (comment) in practice? |
|
Has there been any discussion/arguments for why this is a good idea? Didn't see any in either issue or thread. Typically it's pretty strongly assumed that polyfills has already happened and that it is safe to cache any host environment objects in the module initialization pass. E.g. most polyfills we might rely on. Why isn't that enough in this case? It's nice to cache these locally so that the linking is strong instead of having to do a global map look up on every invocation. In the Prepacked bundles we might want to cache this eagerly as part of the wiring up (and certainly in a wasm/asm bundle). |
|
The main issue is that the polyfill we recommended (patching global) doesn't work for people setting up Enzyme adapter because they do it earlier. I guess we should just change our recommendation to some small package people can import. |
|
I have to say I jumped on this because I spent an annoying amount of time the other day trying to get the warning to go away in jest only to realize it was enzyme, and had to awkwardly change imports to require to avoid hoisting issues or Yet Another Jest Setup File I think it's a small thing but it caught me and was a waste of time so seemed like a easy little win |
There was a problem hiding this comment.
IMO Enzyme should also not be using dynamic dependency injection since that leads to even more of these quirks but kind of unrelated here. Seems to me that this is an ES modules issue rather than an Enzyme issue, no?
Seems like this would happen with any ES module system that tries to do polyfilling in the same file as imports.
I tend to prefer making this early errors so that we deal with it now rather than when we later want to cache these earlier. It may or may not be fine in this case but if the best practice is different, why not enforce the correct practice early than ad-hoc one-by-one?
Additionally, it is usually nicer to fail early. On many React apps that might be early but some apps only render conditionally or lazily so you wouldn't notice it in those until later which could be confusing too.
Separately I'm also concerned that the polyfills used in the examples are not high fidelity, meaning they don't pass a frame time stamp so that will kill the polyfill and not work with async mode enabled.
We should probably also check for window.postMessage since we use that as well in the polyfill.
|
I figure we decided not to do it. |
fixes #11114