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

Fix production crash #11286

Merged
merged 2 commits into from Oct 19, 2017
Merged

Fix production crash #11286

merged 2 commits into from Oct 19, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Oct 19, 2017

Fixes an embarrassing production crash I introduced in #11284. I misunderstood what this fbjs function does.

Verified this fixes it by trying the rebuilt bundle on FB.com in production mode.
Also by newly added regression test (which fails on master).

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.

Oh, whoops 😮

@@ -211,6 +211,28 @@ describe('ReactDOMProduction', () => {
);
});

// Regression test verifying that trying to access (missing) component stack doesn't crash.
it('should throw on children for void elements', () => {
const errorCode = 137;
Copy link
Contributor

Choose a reason for hiding this comment

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

You could put the entire error string into this var since it's duplicated below?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I guess.. Just copy paste from above tests. I don't really care. I'll have to get back to this and find another way to test it when I enable production testing of bundles.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, no big deal. Just seemed pretty arbitrary the way it was currently setup.

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 agree. I just don't want to spend time here because I'll delete those tests anyway.

@gaearon gaearon merged commit 1740f30 into facebook:master Oct 19, 2017
@gaearon gaearon deleted the fix-prod branch October 19, 2017 18:18
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