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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove disableNewFiberFeatures flag #10585

Merged
merged 1 commit into from Sep 1, 2017
Merged

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 31, 2017

馃敟 馃敟 馃敟 馃敟 馃敟

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

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

I don't understand why we had useFiber changes in the tests?

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 31, 2017

These tests now fail with Stack because it has a different message. I was too lazy to fix up the message for both cases since Stack is not used.

@sophiebits
Copy link
Collaborator

I didn't think this commit affects Stack though. I must be missing something.

@gaearon
Copy link
Collaborator Author

gaearon commented Sep 1, 2017

The flag defaulted to false in tests.
So the tests that explicitly wanted the new code path to run had to opt in.

The message when returning undefined from component is different depending on the flag.
Some tests (that didn't opt into the new code path) asserted the old message gets thrown.

Now that there is no flag, and new code path always runs, these tests assert on the wrong message.
I changed them to assert the new message.
However, the Stack code still always emits the old message.

I could write conditionals like

expectDev(message).toContain(
  useFiber ?
    '...' :
    '...'
)

but I figured it's easier to gate the whole test (and later remove the gate).

What does this change have to do with useFiber flag though?
Well, now the only code path with old message is Stack one.
Fiber now always uses the new code path.

@gaearon gaearon merged commit de05d2e into facebook:master Sep 1, 2017
@sophiebits
Copy link
Collaborator

Ahhhhh sorry I missed that the message changed.

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.

None yet

3 participants