Skip to content

Put createRoot export under a feature flag#11426

Merged
acdlite merged 1 commit intofacebook:masterfrom
clemmy:feature-flag-createroot
Nov 2, 2017
Merged

Put createRoot export under a feature flag#11426
acdlite merged 1 commit intofacebook:masterfrom
clemmy:feature-flag-createroot

Conversation

@clemmy
Copy link
Contributor

@clemmy clemmy commented Nov 1, 2017

As per the request of @acdlite . This feature is not ready to ship yet.

@gaearon
Copy link
Collaborator

gaearon commented Nov 1, 2017

This only seems to disable tests, not export itself?

@clemmy clemmy force-pushed the feature-flag-createroot branch from 0fbab4a to 13c67ad Compare November 2, 2017 00:53
@clemmy
Copy link
Contributor Author

clemmy commented Nov 2, 2017

Not sure how I missed that. 😳

return new ReactRoot(container, hydrate);
},

var ReactDOM: Object = {
Copy link
Contributor

Choose a reason for hiding this comment

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

For my own understanding of Flow, does : Object distinguish ReactDOM's type over what Flow can infer?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess this relaxes the type so we can add things to it later. Actually this is probably not a great pattern and we should stop doing this. Since it probably also relaxes which properties it lets us read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should make sure to remove the : Object when removing the enableCreateRoot flag.

root.render(<div>Hi</div>);
expect(container.textContent).toEqual('Hi');
});
if (ReactFeatureFlags.enableCreateRoot) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@clemmy @gaearon Can y'all explain how this works? Do we run the tests twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what you are asking. My understand is that the tests should only run once, or not at all.

@gaearon
Copy link
Collaborator

gaearon commented Nov 2, 2017

Oops @acdlite is right. If we disable this in the main feature flags we never run these tests.

What we should do instead is to mock the feature flags file in tests. And we need to do the same for fragment tests. Sorry I missed this.

Copy link
Collaborator

@gaearon gaearon left a comment

Choose a reason for hiding this comment

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

Need to mock feature flags instead to actually run those tests. And do the same for fragments.

@gaearon
Copy link
Collaborator

gaearon commented Nov 2, 2017

Please see #11430.

@clemmy clemmy force-pushed the feature-flag-createroot branch from 13c67ad to b3f6051 Compare November 2, 2017 19:27
@acdlite acdlite merged commit 1e35f2b into facebook:master Nov 2, 2017
Ethan-Arrowood pushed a commit to Ethan-Arrowood/react that referenced this pull request Dec 8, 2017
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.

5 participants