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

SSR: getDataFromTree works only for the first nested level of components in the tree #250

Closed
graphan opened this Issue Oct 7, 2016 · 20 comments

Comments

Projects
None yet
3 participants
@graphan
Copy link

graphan commented Oct 7, 2016

Steps to Reproduce

I have the following structure of components (you can create own with such a structure):

├── App
   ├── Header
   |     |── Menu*
   |     |── Lang*
   |
   └── Page*

Components with asterixs are connected with graphql queries. Unfortunately, only data for Page component are fetched during Server Side Rendering.
It is depicted in this project. Execute these steps to run the project.

Buggy Behavior

getDataFromTree does not work as expected in this sample.
GraphQL query was executed only for Page Component. It can be checked in window.__APOLLO_STATE__. There is no execution for Menu and Lang Components.

Expected Behavior

getDataFromTree should execute queries for all component tree structure, not only the first nested level of components.

Version

  • apollo-client@0.4.20
  • react-apollo@0.5.7

@graphan graphan changed the title SSR: getDataFromTree works only for the first level of components in the tree SSR: getDataFromTree works only for the first nested level of components in the tree Oct 7, 2016

tmeasday added a commit that referenced this issue Oct 17, 2016

Added an explicit nested queries test
(trying to replicate #250)
@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 17, 2016

Hi @graphan -- it is possible to boil it down to a simpler reproduction?

Certainly I would expect getDataFromTree to work in your case. A quick inspection of your code makes it look like it should work.

I wasn't able to run your example above -- in which directory am I supposed to run npm install?

It would be easier to run an example against a simpler server;

a) ideally as a test, you could add one to this file: https://github.com/apollostack/react-apollo/blob/master/test/react-web/server/index.test.tsx

b) or against a known graphql server, for instance:
i) you could use https://github.com/apollostack/frontpage-api OR
ii) http://graphql-swapi.parseapp.com

tmeasday added a commit that referenced this issue Oct 17, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 17, 2016

Interestingly I added a test to try and replicate (6512735) and although it works normally, it fails when running w/ NODE_ENV=production. (See #237)

Are you doing that?

@graphan

This comment has been minimized.

Copy link
Author

graphan commented Oct 18, 2016

@tmeasday, thank you for the reaction.

Maybe, I misled you posting execution steps above (already deleted). Please, just execute these steps.

This server was set up in order to prove that react-apollo can be used against every GraphQL API (even the one created by graphql-php). All is set up to be run almost automatically by vagrant, ansible, docker or.... manually... what you wish :)

Yes, I use NODE_ENV=production, but the interesting fact is that when I am not using it, the problem still occurs.

The other thing that might be important is that I use graphql@0.4.14, because it works seamlessly with graphql-php. Might the old version of graphql be the issue?

@graphan

This comment has been minimized.

Copy link
Author

graphan commented Oct 18, 2016

@tmeasday, currently you need to set up sample data manually, so please wait until I do it all automatic. I will let you know.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Oct 18, 2016

@graphan it might be clarifying to try and reproduce the problem against a simple server like the frontpage-api, hopefully it'll make it clearer where the issue is.

@graphan

This comment has been minimized.

Copy link
Author

graphan commented Oct 19, 2016

@tmeasday, the server has been already updated. You can launch it using one/two commands on every OS. I tried to do it as automatic as possible. You can try it.

Meanwhile, I will try to reproduce the issue against any simpler server.

@graphan

This comment has been minimized.

Copy link
Author

graphan commented Oct 19, 2016

I have already applied your hints. I just forked react-apollo-example and made it compatible with: https://github.com/apollostack/frontpage-server.

Fork here: https://github.com/graphan/react-apollo-example
The only changes that I applied. Please, don't laugh at Readme :)

Unfortunately, the problem still occurs. You can open web console and check __APOLLO_STATE__.
Two things there:

  • query is only executed for Page Component
  • query is executed two times (I think that it is due to using Redux and updating state, but how to avoid it?)
@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Oct 31, 2016

I got the same issue, works fine while developing, but in my production build there is only one level of data fetched.

Strangely this doesn't happen locally in production mode (mac os), only on the server (heroku). I'm using yarn, so the exact same dependency versions are installed on the server. I have no clue why the app behaves differently. I already cleared my local node_modules and reinstalled, with the same result. Works locally, doesn't work on the server.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 3, 2016

@bkniffler can you tell if you are getting the production version of React locally? You can tell easily by e.g. passing incorrect prop types; does it log a long warning?

@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Nov 3, 2016

Yes, I just checked it and I'm certain to be running production version. Especially, since react throws a warning if minified and non production. Haven't you been able to reproduce the issue on your side?

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 4, 2016

I guess I am wondering about the odd behaviour you are seeing when the issue doesn't happen locally. I've seen the bug in production mode in OSX.

@tmeasday tmeasday referenced this issue Nov 7, 2016

Merged

Refactor ssr #313

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 7, 2016

This is probably fixed on refactor-ssr if you are game to give it a try.

@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Nov 7, 2016

I got multiple problems when using refactor-ssr branch.

Using the current react-apollo/server.js makes all these problems disappear again.

@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Nov 7, 2016

I'm revisiting this bug (with react-apollo master branch), but I can't find anything that would explain this behaviour. I've inserted some console.logs into my code and its clear that on the server, render is only called once on the components, which results in only one level of data being resolved, while locally on my workmachine render is called multiple times (at least 4 times). Still talking about the exact same code running on both machines, dunno where the differences come from.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 8, 2016

Hi @bkniffler; thanks for investigating further.

It sounds like the new branch has a few rough edges, thanks for helping me sort them out.

A) How do you get null elements into walkTree?
B) How did you work around the context issue w/ RR? What was it being caused by?

If you are feeling keen, perhaps you could submit a PR to the refactor-ssr adding some simple tests to https://github.com/apollostack/react-apollo/blob/d127d02c0b938ca40d2da953ab133c45d310a402/test/react-web/server/index.test.tsx#L35 that exhibit the behavior? (and we can work to fix them). Otherwise a little more detail here and I'll try to get it going.

On the last point, getDataFromTree no longer returns the context (see the top comment in #313). If you want the state, you can just call client.store.getState().apollo or the like. I'll document this better soon.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 8, 2016

Still talking about the exact same code running on both machines, dunno where the differences come from.

@bkniffler the only thing I'm aware of is the issue mentioned here: #259 where the old SSR code did something funky that threw an exception in development, and led to a different code path from production.

But you said that you were running React in production locally so I can't really suggest anything else. Is it possible that on the server the queries are not returning the same thing? (Perhaps an error?)

@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Nov 8, 2016

Yesterday I was totally focused on trying to fix the master branch after being frustrated with the refactor-ssr one, but nothing worked. After your messages I thought I'd give the new branch a try and.. it works perfectly now, on the server and locally with react-router, code-splitting and nested data to the lowest level!

It was actually quite easy to fix the refactor-ssr branch. Here is a PR: #317

On a sidenote: I'd find it better if renderToStringWithData would return the markup and the data, even though it works fine to pull the state manually from client. But the name renderToStringWithData sounds like it should return the data (like it was before).

@graphan

This comment has been minimized.

Copy link
Author

graphan commented Nov 12, 2016

@tmeasday or @bkniffler , could you tell me how to try refactor-ssr in the easiest way?
I mean that I added something like this to my package.json:

"react-apollo": "apollostack/react-apollo#refactor-ssr",

However, this way is not correct. I cannot import react-apollo:

import { graphql } from 'react-apollo';

How to do it seamlessly?

@bkniffler

This comment has been minimized.

Copy link

bkniffler commented Nov 12, 2016

@graphan Just create a file 'apollo-server-fix.js' or similar inside your project and insert the apollo server code (https://github.com/apollostack/react-apollo/blob/master/src/server.ts) and import that script to call getDataFromTree/renderToStringWithData. For the sake of comfortability and if you don't use typescript, here is a compiled version of the server.ts file that you can copy&paste: https://github.com/olymp/olymp/blob/master/graphql/apollo-server-fix.js

Since the patch just concerns this one file and function (getDataFromTree/renderToStringWithData), it should be enough as a temporary fix until the branch gets merged and published.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Nov 14, 2016

This is now out in 0.6.0. I think it fixes this issue.

@tmeasday tmeasday closed this Nov 14, 2016

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