Skip to content

Conversation

@bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jan 3, 2017

This test previously returned undefined from render() to cause an error. Fiber allows this though so the test was failing. I've updated the test instead to trigger a different error that both Stack and Fiber share in common.

This test previously returned undefined from render() in order to cause an error. Fiber permits this. I've updated the test instead to trigger a different error that both Stack and Fiber share in common.
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.

Fiber should not allow this (unless it's an intentional change, in which stack also should).

@acdlite
Copy link
Collaborator

acdlite commented Jan 3, 2017

What's the argument against treating undefined from render as empty?

@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2017

Related: #8648 where I implement the opposite. 😄

@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2017

That said I think we should have separate tests covering this situation (undefined from render). This particular test is about error codes, not about behavior.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 3, 2017

This particular test is about error codes, not about behavior.

Agreed. I don't know the backstory about why ReactChildFiber treats null and undefined the same. That didn't seem to be the focus/intent of this test though.

@sophiebits
Copy link
Collaborator

From https://fburl.com/r6pdr0a8 (FB-internal link), @sebmarkbage said in 2014:

This should at least be == null. Maybe even === ? Letting undefined be passed might hide bugs.

false (and even true) is allowed to be passed to multi-child so maybe it should be allowed here too? Allows shorthand for conditional rendering but messes with static types. We should be consistent.

return x && <div>{x}</div>;

Empty strings and the number zero should be rendered as strings. Not empty component.

Mostly I think we thought it was safer to disallow undefined so that forgetting to return anything is an error. I'm okay with changing it if we want to but we can't just have random stack/fiber divergences.

@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2017

The existing behavior (which Fiber currently doesn't pass) is covered by this test.
So this change seems safe to me as it tests different behavior.
We'll fix the divergence later but it's unrelated to this PR.

@sophiebits
Copy link
Collaborator

Why not leave this test alone and just fix both tests by adding the invariant?

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 3, 2017

it was safer to disallow undefined so that forgetting to return anything is an error

Why not leave this test alone and just fix both tests by adding the invariant?

That's a compelling argument. 👍

@gaearon
Copy link
Collaborator

gaearon commented Jan 3, 2017

Then let's get #8648 reviewed!

@bvaughn
Copy link
Contributor Author

bvaughn commented Jan 3, 2017

Going to close this PR in favor of #8648 then. Thanks!

@bvaughn bvaughn closed this Jan 3, 2017
@bvaughn bvaughn deleted the fix-production-minified-error-test branch January 3, 2017 20:21
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