-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Move ReactFiberTreeReflection to react-reconciler/reflection #11659
Comments
can i work on this one? I may need some time to study it first~ |
Sure, but please don't disappear and keep us updated. :-) |
no problem! will keep you posted! |
Add me in a queue. :) |
@gaearon I think i am on the right track. Currently, the build process is using rollup to bundle the imports into a single js file. Instead of bundling together, we want it to be a dependency and load it using require after the bundling process. I notice that ReactFiberTreeReflection is also used in packages like react-dom, react-native-renderer, etc, does that mean i need to add the dependency in their package.json as well? I may also need to experiment with the "external" options in bundle.js to see if it can correctly bundle using require statements. |
Yes, but only for
I think our build process should automatically treat something as an external for CommonJS builds if it's in |
@gaearon, the "ReactFiberInstrumentation" cannot be found when trying to run yarn test-build after making react-conciler as a dependency because it does not exists as a flat bundle when loading "ReactNoop". Should i also convert this into another flat bundle? Or export it in "ReactFiberReconciler"? |
I forgot about that one. Let's just remove that import for now? It's only used by a debugging tool that I haven't updated for months anyway. We'll figure out how to fix it later. |
@gaearon same as ReactFiber, ReactFiberUpdateQueue in ReactNoop. Should i make a flat bundle for them to fix those direct imports? |
These are type imports, not real imports. They won't affect the tests or the build. |
gotcha, those are for flows. |
* Move ReactFiberTreeReflection to react-reconciler/reflection #11659 * Use * for react-reconciler We don't know the latest local version, and release script currently doesn't bump deps automatically. * Remove unused field * Use CommonJS in entry point for consistency * Undo the CommonJS change I didn't realize it would break the build. * Record sizes * Remove reconciler fixtures They're unnecessary now that we run real tests on reconciler bundles.
Fixed in #11683. |
* Move ReactFiberTreeReflection to react-reconciler/reflection #11659 * Use * for react-reconciler We don't know the latest local version, and release script currently doesn't bump deps automatically. * Remove unused field * Use CommonJS in entry point for consistency * Undo the CommonJS change I didn't realize it would break the build. * Record sizes * Remove reconciler fixtures They're unnecessary now that we run real tests on reconciler bundles.
We should:
ReactFiberTreeReflection
to be exported fromreact-reconciler/reflection
entry point (similar to howreact-dom/test-utils
entry point is set up).react-dom/test-utils
is set up inbundles.js
).ReactFiberTreeReflection
directly to do it throughreact-reconciler/reflection
instead.react-reconciler
a dependency ofreact-noop-renderer
inpackage.json
. This should ensure the reconciler doesn't actually get bundled withreact-noop-renderer
, and instead stays arequire()
call. Then the interaction of these two packages will actually be tested when you runyarn test-build
(afteryarn build core,noop,reconciler --type=NODE
).This task is not friendly to beginners. It requires an understanding of the relationship between different packages, and a willingness to research and experiment with how our build process is set up.
The text was updated successfully, but these errors were encountered: