Skip to content

Only renderers should depend on reconciler code#11281

Merged
gaearon merged 2 commits intofacebook:masterfrom
gaearon:fix-test-utils
Oct 19, 2017
Merged

Only renderers should depend on reconciler code#11281
gaearon merged 2 commits intofacebook:masterfrom
gaearon:fix-test-utils

Conversation

@gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 19, 2017

This addresses #11280.

We forbid TestUtils and ShallowRenderer from bundling the reconciler code.

The build breaks since they depend on some Fiber modules. However those modules happen to be stateless helpers. So I move those to shared acknowledging their nature (just like ReactTypeOfWork already lives there).

We can (and maybe will need to) fix this in a different way by moving them to the reconciler and making them exports on it. Since third-party renderers might need them anyway.

However this is a reasonable solution while we’re untangling dependencies. Again, it doesn’t change how it works—just formally encodes what we already do.

I added some todos for server dependencies on reconciler modules. Should be easy follow up fix.

@gaearon
Copy link
Collaborator Author

gaearon commented Oct 19, 2017

Verified this doesn't change the build except for the comment in the source.

@gaearon gaearon requested a review from trueadm October 19, 2017 14:35
Copy link
Contributor

@trueadm trueadm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

They share ReactDOMFrameScheduling so I moved it to shared.
@gaearon
Copy link
Collaborator Author

gaearon commented Oct 19, 2017

Pushed an additional fix for react-art.

@gaearon gaearon merged commit c625b86 into facebook:master Oct 19, 2017
@gaearon gaearon deleted the fix-test-utils branch October 19, 2017 14:59
Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

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.

4 participants