-
Notifications
You must be signed in to change notification settings - Fork 47.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Split ReactNoop into normal and persistent exports #12793
Conversation
Details of bundled changes.Comparing: 4b2e65d...c26dba7 react-noop-renderer
Generated by 🚫 dangerJS |
|
||
import ReactNoopShared from './ReactNoopShared'; | ||
|
||
const ReactNoopPersistent = ReactNoopShared( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you going to swap this out or keep the instantiation mechanism?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want to keep instantiation. I think it works fine and in this case I don’t see downsides (we don’t plan to make this one inlined).
@@ -0,0 +1,582 @@ | |||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I'm not a big fan of "Shared" as a name. It's like utils. Doesn't say anything. Don't really know where this is going yet so hard to find a better name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can make it createReactNoop. No plans to do anything else with it.
This complements #12791 although doesn't depend on it.
While working on #12792 I realized that we call
ReactFiberReconciler()
twice in the same file (ReactNoop
), once with a mutating and once with a persistent mode.If we make the host config resolved through imports, we can't change what one copy "sees" without changing what another copy "sees" unless we reset modules in the middle. That's what I do with ReactDOM and ReactART in #12792, but I can't
resetModules
between twoReactFiberReconciler()
calls inReactNoop
because they don't happen inside the test—they happen inReactNoop
itself.With this change, we clearly separate
react-noop
andreact-noop/persistent
, can remove the awkwardrenderToPersistentRootWithID
method in favor of normal render, and avoid the problem.Note: this change isn't necessary because we want to inline ReactNoop host config itself (actually I want the opposite: to keep using it for testing the custom renderer infra). But it is necessary because
ReactPersistent-test
won't work in a world where the reconciler modules import host config from a file because we don't get a chance to change what this file points to midway while executingReactNoop
. In practice we don't even use mutating and persistent versions ofReactNoop
in the same test file even once.