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

[Fiber] Relax test about rendering top-level null #8640

Merged
merged 1 commit into from
Dec 27, 2016

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Dec 25, 2016

It asserts error but this is no longer supposed to be an error in Fiber.

Copy link
Contributor

@aweary aweary left a comment

Choose a reason for hiding this comment

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

LGTM

@gaearon gaearon merged commit 38ed61f into facebook:master Dec 27, 2016
var div = document.createElement('div');
expect(function() {

if (ReactDOMFeatureFlags.useFiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is @spicyj ok with this divergence? Seems like it would be pretty easy to start relying on this, even accidentally.

var div = document.createElement('div');
expect(function() {

if (ReactDOMFeatureFlags.useFiber) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@acdlite is going to add a flag for the old behavior which should throw this same error.

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 was going to start working on that—but happy to have @acdlite fix it. I didn't do it initially because I thought we're going to introduce a flag to disable all Fiber features including frags, and do it in a single pass.

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