Apollo 2 - Improvements

eric-burel commented 5 years ago

Waiting for a fix in a 3rd party lib

Can't fix/improve those parts right now, would need improvements in Apollo, graphiql and GraphQL Playground.

[X] Populate the Authorization header when using GraphQL PlayGround with localStorage['Meteor.loginToken]. See https://github.com/prisma/graphql-playground/issues/510 for global Headers (right now we have to create an initial tab to setup headers) setup. The best solution seems to be using cookies. Also, in GraphiQL, we used the passHeader option that allowed access to the localStorage, but it was awfully hacky. See https://github.com/prisma/graphql-playground/issues/849 Note: Apollo 2 Chrome plugin provides a GraphiQL tab out of the box. Edit: maybe using cookies would be the solution? => it works if you are authenticated through the app and open graphQL only afterward, so it's ok
[ ] Change endpoint for GraphQL Playground. See https://github.com/apollographql/apollo-server/issues/1908 and https://github.com/apollographql/apollo-server/pull/1974 (pending PR so it should be implemented soon).
[ ] Allow to override apollo server context (can be either an object or a function to compute context from req, so handling it is not trivial)
[ ] Load GraphiQL as an official middleware. See https://github.com/graphql/express-graphql/issues/113. Have to use a hackish code inspired from https://github.com/eritikass/express-graphiql-middleware until there is an official alternative.

Possible improvements

[ ] migrate away from Apollo Link State (not much used yet anyway) : https://www.apollographql.com/docs/react/essentials/local-state#migrating
[ ] deprecate webAppConnectHandlersUse patch that changes middlewares order and makes /graphql requests fail (but before that, check why we needed this patch in the first place)
[ ] create a callback to enhance the initial context
[ ] create a callback to enhance the context on every request. Right now we have a customContextFromReq option (which is not really public), but that would make more sense to use a callback
[ ] handle SSR errors. I've added a try/catch around renderToStringWithData, which triggers the SSR render, but the error it produces are still a bit cryptic, you can't get which component is faulty
[ ] Investigate apollo-link-state asynchronicity. It seems to be async, even though it calls a local cache (so I would have expected to be sync), so the returned values can be undefined even when setting a default. See withMessages for example: messages can be undefined during loading.
[ ] Add safeguards to avoid this issue (making introspection queries async by accident, while they must be kept sync) https://github.com/apollographql/apollo-server/issues/1935
[ ] Create a specific vulcan:apollo package? This would make it easier for future improvements.
[ ] Fix #2111 (Form SSR/withSingle HOC is broken)
[ ] integrate advanced features of vulcan:routing (options, callbacks, etc.)? Some of them are now unnecessary, to be tested
[ ] write. tests. many tests.
[ ] handle errors. For the moment if the render fails we only display an error client side. In production you'd want the error to be caught and fallback to client side rendering, with maybe a console.warn (to be discussed). In dev mode you'd like the error to print nicely. Try for example to trigger an error in the default_resolvers, it will be hard to debug. I've added the ErrorLink server side to Apollo client but I don't know what it should do. Edit: ErrorLink does not work server side (does not import), it is meant for client errors.
[ ] Investigate using Data Sources instead of DataLoader to pass the collection to the resolvers. We agreed that keeping the current api (context.YourCollection) for now is a good idea. We can introduce Data Sources in a next version, while keeping the current syntax as a legacy for a while, there is no emergency. Also beware, the update might be trickier as it seems as there is caching involved.
[ ] Using State Link is very verbose, any input welcome to simplify syntax (we should especially try to rely on a 3rd party lib for this). See withMessages for example.
[ ] Check server side cache. Try the example-forms, fill a new Customer but do not add an address: you'll get crappy error messages about the cache (cache.readData is not defined) + error handling is broken. Sometimes the error is correct, sometimes it's weird. If you add an address it will correctly save the data however.
[ ] WebApp (Express) middlewares can't seem to enhance the request object passed to the meteor/server-render onPageLoad callback. The middleware runs correctly, but the sink.request object is still the initial request. See https://github.com/meteor/meteor-feature-requests/issues/174#issuecomment-441047495 for more details (example with Cookies)

Done so far

[X] Fixed user and userId in context. Now we use meteor/apollo getUser, which encapsulate the logic that was need in v1 (checking the token and so on). I noticed that sometimes the authorization header is "null" (as a String, not the null object`, because a JSON was stringified) though, this is not very clean, maybe we can improve the client part.
[x] Take a look at https://github.com/VulcanJS/Vulcan/issues/1662 and handle CORS correctly
[X] Allow to configure the server. See for example PR #2147 (and the relevant code in Apollo2). See registerApolloServerOptions and registerApolloApplyMiddlewareOptions methods.
[X] create an Apollo client for the server (it fetches all the data it can during SSR). It relies on SchemaLink for the moment. Thus the server does not need to request himself using HTTP, it directly calls resolvers.
[X] render the App as HTML server side.
[x] Reenable inject_data. This allow to pass down data from the SSR rendering to the client, for example to fix #2218 by comparing the URL used during render and the client URL
[X] populate the document using Helmet
[X] inject the Apollo state
[X] return the rendered HTML on requests Previously we relied on dynamicHead, that is then used by the template-web.browser.js file of _boilerplate-generator. Do we still need this? Instead I use meteor/server-render features to append content to the request body. This first set of features is enough to render a basic page like the getting-started first steps. Following features allow more complex usage, based on the example-forms app.
[X] handle Apollo Link State server side. The stateLink API is now available client side and server side.
[X] Define context of the SchemaLink, which is in turn passed to the resolvers and is necessary to actually get data. This context is then used in 2 places, as the context option of the server (using for each query), and as the context option of the SchemaLink (used during the initial SSR render). It is recomputed for every request.
[X] Cookies. Could be improved though, as universal-cookies middleware does not seem to be taken in to account (see issue with meteor/server-render and meteor/webapp below.
[X] Add auth with Cookie. On first page load, the Authorization header can't be set, so you can't authenticate. Solution is to rely on a cookie instead, and then only on the Authorization header when it is set. The cookie is updated on client startup based on the localStorage token. I uncommented existing code in auth.js and updated it, it seems fine.
[X] Created a vulcan-redux package. It allows legacy app to work by simply loading this package and imporing addAction, addReducer etc. directly from there. It allowed me to test the router.xxx.wrapper callbacks.
[X] router.server.wrapper: allow to wrap the App component, for example to add a Redux store. Added to AppGenerator.jsx (untested though).
[X] router.client.wrapper: same but client side
[X] router.server.postRender (used to injectJSS for example): now takes the sink object + context as param instead of req/res. vulcan-material-ui will need a very small update to handle this breaking change, see https://docs.meteor.com/packages/server-render.html
[X] Added a vulcan:styled-components independent package
[X] Other router.server.preRender, router.server.html can be safely ignored, at least for the moment. They are not used in major packages/open source Vulcan example. wrapper and postRender already provides enough flexibility to add a complex lib such as Styled Components or Redux, including SSR.
[X] Fix meteor_login_token cookie issue. The cookie must be set everytime the localStorage token is updated, see auth.js
[X] Allow to register custom links/terminating links (used by vulcan:files for uploading)
[x] Allow disabling SSR (one the server it's a one liner, on the client we need to prevent rehydratation I think)

SachaG commented 5 years ago

Pretty amazing. I feel like you've almost completely refactored Vulcan's back-end! Really excited to work with the new Apollo packages moving forward :)

SachaG commented 5 years ago

Here's some notes on what I had to do to upgrade properly:

update apollo-client
update apollo-server-express
update graphql
update react-apollo
update react-router
update react-router-bootstrap
update body-parser
update express
npm i graphql-voyager --save-dev
npm i apollo-server
npm i apollo-cache-inmemory
npm i apollo-link-schema
npm i apollo-link-state
npm i apollo-link-error
npm i react-router-dom
npm i apollo-link-watched-mutation
npm i apollo-link-http

React Router 4

link is now imported from react-router-dom
no more props.location.query

Issues

tmeasday:check-npm-versions -> wrong version of apollo meteor package

eric-burel commented 5 years ago

A few additional notes for RR4 update:

IndexLink no longer exists. Use <NavLink exact ... > instead
Link and NavLink are now imported from react-router-dom (instead of react-router)
Only NavLink tolerate styling, eg with activeClassName prop. Link are now simple <a> tags as a default.
onlyActiveOnIndex can be replaced by exact.
this.props.router.isActive(path) becomes matchPath(this.props.location.pathname, { path }) (matchPath is provided by react-router)

SachaG commented 5 years ago

Some more issues I've noticed:

[x] Event tracking is broken:

TypeError: getRenderContext is not a function
    at trackInternal (internal-client.js:6)
    at track (events.js:21)

[x] queryOne/runGraphQL is broken? (Error: Expected undefined to be a GraphQL schema)
[ ] GraphQL errors don't appear in the terminal (only in devtools network tab)
[x] i18n doesn't work for GraphQL content
[ ] current user is not logged in after hot code reloading

VulcanJS / Vulcan