search

ctrlplusb / react-universally

A starter kit for universal react applications.
MIT License
1.7k stars 244 forks source link

Feature/apollo #469

Closed strues closed 7 years ago

strues commented 7 years ago

@This is a rewrite of #367.

I saw there was interest in this again still so spent a few hours tonight. Everything works, but I've got a few things left to do.

New:

diondirza commented 7 years ago

really looking forward to have this feature branch get merged

strues commented 7 years ago

Cleaned up and updated dependencies.

Early this morning, I encountered the following error relating to react-apollo and react-dom:

TypeError: Cannot read property 'ReactCurrentOwner' of undefined
 at Object.<anonymous> (~/node_modules/react-apollo/node_modules/react-dom/cjs/react-dom-server.development.js:3534:36)

If for whatever reason you encounter this error, checkout https://github.com/apollographql/react-apollo/issues/826 where some discussion is held and for a solution.

tl;dr solution: 

cd node_modules/react-apollo
npm remove react-dom

I've never run across this issue in any of my other Apollo projects. It's isolated, but not isolated enough. Yet.

I removed react-jobs because, it's not necessary with GraphQL/Apollo data fetching. We're basically close to a merge at this point.

Let me know any feedback. If someone @ctrlplusb @diondirza @sergiokopplin doesnt mind pulling this down and double checking everything works, we can get this merged today.

ctrlplusb commented 7 years ago

Frikken awesome @strues!

Totally agreed on removing react-jobs - definitely not needed in this context.

I won't have time to give it a proper review so would defer this responsibility to any of the others who may have a spare moment.

diondirza commented 7 years ago

@strues that error happened on my local, thanks for pointing out the solution and now runs perfectly. Except will find error with yarn check command, I think that need to be solved. Anyway would be glad if there are some documentation about apollo implementation in the sources.

strues commented 7 years ago

Additional documentation added. Shall I resolve the merge conflicts and merge this baby?

diondirza commented 7 years ago

Please merge this baby @strues. Also maybe you could include graphql-tag/loader to preprocess import from .graphql files, that would be nice.

strues commented 7 years ago

Merged.