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

Children of Consumer without new line gives "TypeError: render is not a function" #12241

Closed
ryepesg opened this issue Feb 17, 2018 · 10 comments
Closed

Comments

@ryepesg
Copy link

ryepesg commented Feb 17, 2018

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

I get a confusing error when not using a new line in the children of a Consumer un the new Context API:
https://codesandbox.io/s/13n733xp5j

Error:
selection_108

Fix:
selection_109

What is the expected behavior?

Component just getting redered

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

16.3.0-alpha.1 and 16.3.0-alpha.0, Chromium 63, Archilinux. React 16.2 didn't support the new Context Api.

@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2018

We should show a nicer error message. I don't agree this should just render (it's too ambiguous, technically you have a text node there). But the message should make more sense.

@elbaumpj
Copy link

@gaearon I'd like to jump in on this!

@aweary
Copy link
Contributor

aweary commented Feb 18, 2018

@elbaumpj it's all yours!

@elbaumpj
Copy link

Would someone mind pointing me in the right direction as to where I might find the relevant logic for this issue? I figured it would be in ReactContext.js, but I'm having trouble figuring out where the actual error message is being generated.

Also, I wonder what the new message should say. Something like "The Consumer component's children must return a React element (JSX)"? To be honest, I'm not exactly sure why Consumer demands a new line for it's children.

@aweary
Copy link
Contributor

aweary commented Feb 20, 2018

@elbaumpj you can find the call site using the stack trace from the error thrown in the codesandbox example posted by @ryepesg. The context consumer's render callback is called here:

const render = newProps.children;
const newChildren = render(newValue);

We should add an invariant call before calling render that checks that render is actually a function.

To be honest, I'm not exactly sure why Consumer demands a new line for it's children.

It doesn't actually require a newline, it just requires that you only render a single child that is a function. In this case, the space before the function created a text node. So when you do:

<Consumer>{() => "foo"}</Consumer>

children will just be that function that returns "foo". But if you insert a space:

<Consumer> {() => "foo"}</Consumer>

children will now be [" ", [Function]] because the space is meaningful. Trying to call that array of children as a function is what causes it to throw. The newline fixes it because a newline doesn't create a new text node. Does that help?

Also, I wonder what the new message should say. Something like "The Consumer component's children must return a React element (JSX)"?

@gaearon probably has better copy for this, but the gist of it should be something like:

A context consumer was rendered with multiple children, or a child that isn't a function. A context consumer expects a single child that is a function. If you did pass a function, Make sure there is no trailing or leading whitespace around it.

@elbaumpj
Copy link

@aweary that's all super helpful--thank you!

@raunofreiberg
Copy link
Contributor

Took a crack at this at #12267 - hope it's not an issue.

@elbaumpj
Copy link

elbaumpj commented Feb 22, 2018

@raunofreiberg

I don't really appreciate that, I had clearly claimed this issue and just hadn't got a chance to make my PR yet.

As it states in the docs:

If you decide to fix an issue, please be sure to check the comment thread in case somebody is already working on a fix. If nobody is working on it at the moment, please leave a comment stating that you intend to work on it so other people don’t accidentally duplicate your effort.

If somebody claims an issue but doesn’t follow up for more than two weeks, it’s fine to take over it but you should still leave a comment.

@gaearon
Copy link
Collaborator

gaearon commented Feb 22, 2018

Oops, sorry I merged without looking who took the issue :-(
I assumed that the submitter saw this thread and wouldn't try to submit a PR outside the process.

I guess it's fixed now, but @raunofreiberg please don't take claimed issues in the future.

@gaearon gaearon closed this as completed Feb 22, 2018
@raunofreiberg
Copy link
Contributor

Got it. My apologies @elbaumpj.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants