Apollo2 merge devel

[eric-burel]

Almost there!

I've updated the branch to match devel.

What blocks final merge:

• There can be slight regressions, as latest apollo-server.js fixes improvements/fixes will be lost (they are present but commented out). For example Allow user to customize apollo json parser options #2147. Basically we need to be able to pass options to the Apollo Server.
• Mongo_redux may be discarded, it makes example-forums fails

When updating an app the biggest source of breaking change is direct use of RR4, but we can nothing about it so that's okay, it's quite easy and even documented. Also redux related functions must be imported from vulcan:redux now. A few router.XX callbacks are deprecated.

I've updated my comment in #2081 accordingly.

And that should be all 🎉 Most example of the Starter are working as expected. It may need an alpha release to spot other bugs, I may also try to update Awesome Vulcan to check if there are issues I have missed.

[eric-burel]

Concerning currentRoute if I understand your question correctly it's just that the API of RR4 is a bit different and I was not sure which props currently provides it (I guess props.location?), don't hesitate to reenable this. I may also have dismissed stuffs in App.jsx by accident when merging.

webAppConnectHandlersUse is still used, you have an example in the apollo_server2.js file for cookies (this line can be commented btw, it does not work as expected).

[SachaG]

Weird, I'm not sure what's going on but I ended up pushing to this branch: https://github.com/VulcanJS/Vulcan/tree/lbke-apollo2-merge-devel which doesn't appear in this PR.

[SachaG]

Does http://localhost:3000/graphiql not work anymore?

Also, although SSR works I'm running into an issue where every request to http://localhost:3000/graphql times out. Weird.

[eric-burel]

I don't know about the git branch thing sorry, I am not very experienced on this.

GraphiQL should be replaced by Playground indeed, on the /graphql endpoint instead of /graphiql.
But due to the auth issue with Playground, we should add back GraphiQL if possible until its fixed.

The timeout thing sounds weird indeed, do you have more info on how to reproduce? It's just failing in your app? I'll try it again on Awesome-Vulcan tomorrow.

[SachaG]

I think it's probably clearer/safer to keep the playground and/or graphiql on separate URLs than the endpoint? And yeah I think keeping GraphiQL for now would be best, so we don't change everything at once?

For the timeout no, all I can say is that curl http://localhost:3000/graphql doesn't return anything. Would GraphQL errors be logged out as server logs just like before?

[eric-burel]

Ok I'll try that. The /graphql thing with playground is the default option, I think they tried to make things simpler but indeed that could actually be a bad idea. I'll set it up on /graphql-playground instead so things are 100% clear.
I'll also add back graphiQL if possible.

And yes GraphQL errors should appear on the server logs, though I did not investigate error handling yet.

[Apollinaire]

This is awesome!
I'll start using this on a side project too, to find some possible bugs 👍

[eric-burel]

• I got graphiQL back to work! Not easy, it is not meant to work with Apollo 2 at all sadly, so the solution is a bit hackish (copy pasting 1.4 code and from https://www.npmjs.com/package/express-graphiql-middleware)
• GraphQL Playground endpoint is not customizable right now :( there is a pending PR though so it should be fine in a few weeks. This explains your issue with CURL.
• I add registerApolloServerOptions and registerApolloApplyMiddlewareOptions methods to allow customization. This for example allow to change the bodyParser options. Note that we can't rely on settings because some options can be functions (eg error formatting). This is a small breaking change regarding to PR Allow user to customize apollo json parser options #2147 but now the user can fix it at the app level.

So the Mongo-redux thing is the only thing that still provoke issue. I think the best would be to simply deprecate it and change the Starter package that relies on it (example-forms).

I am opening a new issue to document things that can be improved later/need 3rd party fix.

[eric-burel]

There is also those issues left:

• vulcan:core/modules/callbacks: need to reset flash messages on route change
• Error404 handling in App.jsx

[SachaG]

What is the alternative to getRenderContext to get the Apollo client?

before:

import { getRenderContext, getFragment, createClientTemplate } from 'meteor/vulcan:lib';

const { apolloClient } = getRenderContext();

[eric-burel]

The client is created here: https://github.com/VulcanJS/Vulcan/blob/c81f58a91c63d39d7802359d7b2439a4907784d4/packages/vulcan-lib/lib/server/apollo-ssr/apolloClient.js

It takes a computeContext function as a parameter, which is the same as the function used for the apollo server computeContextFromReq (https://github.com/VulcanJS/Vulcan/blob/c81f58a91c63d39d7802359d7b2439a4907784d4/packages/vulcan-lib/lib/server/apollo-server/context.js).

It is used here:

const makePageRenderer = ({computeContext}) => {
  // onPageLoad callback
  const renderPage = async sink => {
    const req = sink.request;
    // according to the Apollo doc, client needs to be recreated on every request
    // this avoids caching server side
    const client = await createClient({req, computeContext});

However with this implementation it's not easy to retrieve the client if you have another use case, so maybe we could write a small helper that gives you the client.

[eric-burel]

This issue appears when running tests or the Vulcan app directly: apollographql/apollo-server#1935. It is triggered when the Apollo server is created. Not yet investigated.

[jimrandomh]

Gave this a try. Ran into issues, worked through some of them (issues and solutions below), still have some more to work through. Note that this is on LessWrong's fork of Vulcan, which is pretty significantly modified, which may have created some of our own issues.

First issue:

TypeError: utilities_1.getIntrospectionQuery is not a function W20190110-16:07:41.284(-8)? (STDERR)     at Object.generateSchemaHash (/home/jbabcock/repositories/Lesserwrong/LessWrong2/node_modules/apollo-server-core/dist/utils/schemaHash.js:12:44)

This went away after I went through my project's package.json raising version numbers of dependencies. In particular I did:

apollo-engine: ^0.5.4 => ^1.1.2
apollo-errors: ^1.4.0 => 1.9.0
apollo-server: ^2.2.7 => 2.3.1
apollo-server-express: ^2.1.0 => ^2.3.1
graphql: ^0.10.5 => 14.0.2

I didn't take the time to isolate which of these was the fix, or to check whether that GraphQL 14-major-version upgrade broke other things.

Second issue: some failing imports due to missing dependencies. Resolved with npm install --save apollo-link-http apollo-link-watched-mutation.

Next I ran into some issues with react-router, which this PR seems to be partially replacing with react-router-dom (I'm not sure exactly what the relationship between those two packages is?) In particular, packages/vulcan-lib/lib/server/apollo-ssr/components/AppGenerator.jsx importsStaticGenerator from react-router, but it doesn't seem to be defined there, resulting in a type error from React. Changing the import to be from react-router-dom works. Similarly, imports of withRouter in packages/vulcan-core/lib/modules/components/App.jsx and a number of other places also need to be switched from react-router to react-router-dom, and I needed to make the same switch in application code.

The withRouter HoC from react-router-dom does not seem to provide the same set of props as the withRouter in react-router. I took out some references to props.routes and props.pathname in our codebase as a temporary workaround to continue testing.

Next up, react-cookie fails with

Error running template: TypeError: context.cookies.addChangeListener is not a function
    at new Wrapper (/home/jbabcock/repositories/Lesserwrong/LessWrong2/node_modules/
react-cookie/lib/withCookies.js:50:23)

I took out the withCookie in App.jsx as a temporary workaround to get further in testing.

At this point I can get a basic render of the front page, albeit without any Apollo-using components populated.

The SSR rendering is missing its material-UI stylesheet (though it gets successfully filled in later). There are two reasons for this. The first is because the router.server.postRender has changed API, and what was req is now named request (when called from renderPage.js). The second is that we use dynamicHead, which was handled in vulcan-routing in an area of code that now seems to be dead code.

After page load the client makes GraphQL queries to the server, which appear to be correct. However, the queries hang until they time out (as do test queries made with curl). Also, less importantly, each query is sent as a separate request, rather than grouped together into one request, as they were before.

[SachaG]

@jimrandomh thanks for the notes! See also this thread which has few pointers: #2171 (comment)

[eric-burel]

Hi @jimrandomh , thanks for taking time to give this feedback. Concerning React Router, those are normal issues related to RR4 breaking API compared to RR3, there are indeed a few things that changes. Basically all stuff specifically related to web are provided by react-router-dom now.

Concerning material ui, I've pushed a PR on vulcan:material-ui, on the apollo2 branch, you can already access the fix (not yet merged though). It's just a 2 liner. Meteor SSR related API has changed a bit and thus we had to introduce this breaking change in Vulcan too.

Please try the apollo2 branch of Vulcan, the latest commit should fix the timeout issue on /graphql, it seems to be related to the way Express middlewares are handled (bodyParser precisely, the order of parsings was sometimes wrong for no reason, it should be fixed now). We will progressively introduce fixes on this branch.

Btw I'll close this PR to avoid confusion, we can discuss this further on Slack or on #2171