Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFix circular dependencies between shared, reconciler, and react directories #19971
+1,002
−950
Conversation
… to push the __SECRET_INTERNALS symbol down to there as well, and remove the workaround for the circular dependency in the setupHostConfig and other places.
…ks workarounds for the circular dependency it needed.
…Fix up a few imports to be local, which reveals that the cohesion here is quite good. Next step is to move a few more files from shared/ into shared/src, for consistency and to eliminate a few import '..' smells.
… ReactFeatureFlags, as I'm wondering if we want to have a directory separate from src/ for that kind of file. (I also wondered if we might want a separate shared/types directory, but moved those anyway.) Nearly all changes are import path changes, with a couple snapshot updated that had implementation directory details in them.
…t *and* tests at every incremental step. This would have better distributed these changes into the appropriate commits.
…uded multiple times in UMD bundles. I'm not sure that's necessary anymore, as I see the workspace is hoisting the package appropriately. Adding the explicit dependencies in the relevant packages to fix the build for those packages, and hopefully simplify the build further.
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit bdc954c:
|
Details of bundled changes.Comparing: 4ead6b5...bdc954c react
react-dom
react-art
react-cache
react-transport-dom-relay
react-noop-renderer
react-reconciler
react-test-renderer
react-native-renderer
create-subscription
react-transport-dom-webpack
react-server
react-debug-tools
ReactDOM: size: -1.2%, gzip: -0.9% React: size: -0.2%, gzip: -0.3% Size changes (experimental) |
Details of bundled changes.Comparing: 4ead6b5...bdc954c react
react-dom
react-art
react-cache
react-native-renderer
react-transport-dom-relay
react-noop-renderer
react-reconciler
react-test-renderer
create-subscription
react-transport-dom-webpack
react-server
react-debug-tools
ReactDOM: size: -1.1%, gzip: -0.7% React: size: -0.1%, gzip: -0.4% Size changes (stable) |
|
Seems like CI is failing? Also, could you please briefly describe the high level approach? |
|
Also, what's with the |
|
It wa
It wasn’t important, I was trying to get congruency with the other modules in the monorepo as I moved things. I’m happy to move them back to reduce the footprint of this PR. |
Yeah that would be better. FWIW it was intentional that it had no
This might also make sense to send as a separate PR so we don't conflate the real changes with types. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Summary
In trying to align the Roblox Lua version of React (Roact), we came across some circular dependencies. After a brief Twitter conversation with @gaearon , we decided to take a crack at untangling them. The change looks large, but it is mostly tweaking import paths after moving files.
While I'm not sure 'shared' is the right place for some of these things, I think that being able to see the shape of things will make it clearer how these things could have reasonable cohesion, encapsulate detail, and not have any cycles.
It felt particularly good to delete some workaround in the rollup/build scripts that were only there to trick tools into not getting stuck on the circular dependencies. I'd like to be able to remove the ReactSharedInternals reference in react/React.js and just export is from react/index.js, but it looks like some of the tooling demands that it stay there. There were other little things I left in place like that, to not unnecessarily disrupt things.
Test Plan
On a MacBook Pro with node 14.12.0, there are a few tests that fail after a fresh checkout. I want to see if those come up in CI as well, or if it's something in my environment. Besides that caveat, ran all test, flow, lint, and prettier targets. As you'll see in the commits, a few of the circular dependencies were purely in types and had to be worked out.