Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Remove assignment of props and context to component in ssr #259

Closed
wants to merge 2 commits into from
Closed

Remove assignment of props and context to component in ssr #259

wants to merge 2 commits into from

Conversation

rewop
Copy link

@rewop rewop commented Oct 10, 2016

This PR fixes #237.

Assignment of props and context to component instance when doing ssr have been
removed, as they create problems in rendering the component when NODE_ENV=production.
The difference of bahavior between production and development is due to the fact that react freezes the component instance when in development.

Logically the assignment was adding props and context of the parent component in the new created component that was given down the recursion call to be rendered making ssr fail in some cases.

@tmeasday
Copy link
Contributor

tmeasday commented Oct 17, 2016

I think maybe something in between is required.

There is a failing test case in this commit: 6512735

The test fails because eventually we get a rendered element like:

<div>{data.loading ? 'loading' : data.currentUser.firstName}</div>

Which renders to a component that looks like:

<div children="James" />

However the props passed to the element (ownProps) look like { data: ... }.

(I think there is some confusion in this file between elements (pre-render) and components (after render)). I feel pretty confused, anyway.


In any case, the issue is that in production, the attempt to override the props of the component works whereas in dev it fails, which leads to the correct behavior. So clearly there is a bug here.

OTOH there are use-cases where it is necessary to set the context on the component it seems (#218).

@jbaxleyiii I'm not sure why we need to set props also?

A first pass at a solution might be to just set them if they aren't already set. Although this seems to be failing the setState test we have in the SSR test file. I'm not sure if that test is correct though.

@tmeasday
Copy link
Contributor

My explanation above was incorrect.

The real problem in that test case is that the Page stateless functional component is rendered incorrectly (as its props are hijacked similarly to what I said above), and so the graphql component is never rendered and the query is not picked up.

@rewop
Copy link
Author

rewop commented Oct 17, 2016

@tmeasday I am glad you are working on this one.

I also felt pretty confused about elements and component when working on it. Also I believe monkey patching the react components is not a good approach, especially if it generates two different behaviors between development and production environments. This may lead to unexpected behaviors (like in my case).

I suggest a proxy pattern to wrap the component instance and add the logic to fetch the queries.

Finally another thought i had is: the context used when creating the component instance should be dependant on the contextTypes assigned to the element. Only the properties defined in contextTypes should be given to the element.

Simone Potenza added 2 commits October 17, 2016 10:57
Assignment of props and context to component instance when doing ssr have been
removed, as they create problems in rendering the component. Logically it was
adding props and context of the parent component in the new created component
making ssr fail in some cases
@rewop
Copy link
Author

rewop commented Oct 17, 2016

@tmeasday I rebased onto master to include your new test

@jbaxleyiii
Copy link
Contributor

@tmeasday did we ever come to a consensus about this?

@tmeasday
Copy link
Contributor

@jbaxleyiii I was/am planning on looking at it but got distracted for a little bit there ;)

@jbaxleyiii
Copy link
Contributor

@tmeasday haha I wonder why ;)

Fwiw vercel/next.js#106 (comment) is probably being impacted as well.

@tmeasday
Copy link
Contributor

This is now fixed on master thanks to #313

@tmeasday tmeasday closed this Nov 14, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SSR fails with NODE_ENV=production
3 participants