Skip to content
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

Merged
merged 3 commits into from
May 14, 2018

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented May 13, 2018

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 two ReactFiberReconciler() calls in ReactNoop because they don't happen inside the test—they happen in ReactNoop itself.

With this change, we clearly separate react-noop and react-noop/persistent, can remove the awkward renderToPersistentRootWithID 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 executing ReactNoop. In practice we don't even use mutating and persistent versions of ReactNoop in the same test file even once.

@pull-bot
Copy link

pull-bot commented May 13, 2018

Details of bundled changes.

Comparing: 4b2e65d...c26dba7

react-noop-renderer

File Filesize Diff Gzip Diff Prev Size Current Size Prev Gzip Current Gzip ENV
react-noop-renderer.development.js -2.7% -8.1% 18.35 KB 17.86 KB 5.1 KB 4.69 KB NODE_DEV
react-noop-renderer.production.min.js 🔺+4.2% 🔺+2.4% 6.35 KB 6.62 KB 2.5 KB 2.56 KB NODE_PROD
react-noop-renderer-persistent.development.js n/a n/a 0 B 17.89 KB 0 B 4.67 KB NODE_DEV
react-noop-renderer-persistent.production.min.js n/a n/a 0 B 6.57 KB 0 B 2.55 KB NODE_PROD

Generated by 🚫 dangerJS


import ReactNoopShared from './ReactNoopShared';

const ReactNoopPersistent = ReactNoopShared(
Copy link
Collaborator

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?

Copy link
Collaborator Author

@gaearon gaearon May 14, 2018

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 @@
/**
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

@gaearon gaearon merged commit 0470854 into facebook:master May 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants