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

Node compatibility #1

Closed
pluma opened this issue Oct 22, 2014 · 9 comments
Closed

Node compatibility #1

pluma opened this issue Oct 22, 2014 · 9 comments

Comments

@pluma
Copy link
Contributor

pluma commented Oct 22, 2014

According to React's component reference, componentWillMount will also be run on the server. This means bad things will happen (namely, it will attempt to access document on the server, which likely throws an exception).

I don't see any reason not to use componentDidMount instead.

EDIT: See PR #2 for why we can't use componentDidMount for this component.

@gaearon
Copy link
Owner

gaearon commented Oct 22, 2014

Makes sense. Would you mind sending a PR?

@pluma
Copy link
Contributor Author

pluma commented Oct 22, 2014

Sure. I'll try it in my prerendering+client setup and send you a PR if I get it working.

@gaearon
Copy link
Owner

gaearon commented Oct 22, 2014

Please check it doesn't break nested titles. (It shouldn't but didMount execute in different order than willMount so there could be some kind of issue)

@pluma
Copy link
Contributor Author

pluma commented Oct 22, 2014

Is there a particular reason you're using addons.Children.only? I can't find any documentation for it and it breaks when using divs for me. If you only want to check whether this.props.children contains more than one component, invariant(!Array.isArray(this.props.children), "error message here") should suffice.

@gaearon
Copy link
Owner

gaearon commented Oct 23, 2014

Can you give me a breaking example?

One of this component's goals is to emit zero DOM and since react doesn't support returning multiple nodes in render, I require it to have one child.

Only always worked for me for this use case.

@gaearon
Copy link
Owner

gaearon commented Oct 23, 2014

@gaearon
Copy link
Owner

gaearon commented Oct 23, 2014

You might have an error if you're using DT as a leaf node. It's suppose to have a child.

@pluma
Copy link
Contributor Author

pluma commented Oct 23, 2014

I found the problem: I was using my git working directory as a node_dependency in the project and had run npm install inside of that, so DocumentTitle was using a different copy of React than the code using it. Children.only seems to be doing identity or inheritance checks at some point, which would obviously fail. Sorry about the noise.

@pluma pluma changed the title Use componentDidMount instead of componentWillMount Node compatibility Oct 23, 2014
@gaearon
Copy link
Owner

gaearon commented Oct 23, 2014

This problem is literally chasing me.

gaearon/react-hot-loader#32 (comment)
KyleAMathews/coffee-react-quickstart#10 (comment)

I guess I'll file it as an issue in React..

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

No branches or pull requests

2 participants