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

Warn about unresolved function as a child #10376

Merged
merged 2 commits into from
Aug 4, 2017

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Aug 4, 2017

Fixes #9577.

@@ -493,4 +493,78 @@ describe('ReactComponent', () => {
' in Foo (at **)',
);
});

if (ReactDOMFeatureFlags.useFiber) {
fdescribe('with new features', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

-f

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 4, 2017

Ran Jest internally with a throw inside the warning function. Didn’t fail any tests.
Seems like this is good to go?

@@ -200,6 +200,16 @@ function throwOnInvalidObjectType(returnFiber: Fiber, newChild: Object) {
}
}

function warnOnFunctionType() {
warning(
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not pass in newChild and put typeof newChild !== 'function' instead of false?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To avoid two extra functions calls in DEV. (It's small but can add up.)

Also I find our warning notation with falsy condition meaning a failure very confusing. I try to avoid it whenever I write warnings, and always use warning(false so it’s clear from outer condition in which case it executes.

Copy link
Contributor

@flarnie flarnie left a comment

Choose a reason for hiding this comment

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

I had one question, but I think this makes sense overall. Good tests! :)

@gaearon gaearon merged commit efa7148 into facebook:master Aug 4, 2017
@bvaughn bvaughn mentioned this pull request Aug 8, 2017
@luixo
Copy link

luixo commented Oct 19, 2018

@gaearon
Is there an option to use function-as-child-component pattern (as it seems it's not discouraged by react to use it) and not get the warning?

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.

[Fiber] returning functions from render does not throw
5 participants