Skip to content
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

Apollo2 withMessages #2089

Merged
merged 3 commits into from
Sep 28, 2018
Merged

Apollo2 withMessages #2089

merged 3 commits into from
Sep 28, 2018

Conversation

eric-burel
Copy link
Contributor

@eric-burel eric-burel commented Sep 27, 2018

I updated the withMessage HOC to use the new Apollo Link State API instead of Redux. It can serve as an example too.
Updating react-apollo to v2 seems to fix issues with undefined results in the graphql tag.

  • The syntax with Apollo Link State is far more verbose than with Redux. It seems that the Apollo devs are waiting from more feedback from the community before they provide helpers. There are some redundancies between the gql queries and the mutations.
    So I think it would be relevant to implement some helper functions, maybe smth similar to Redux or Rematch API, or even maybe to generate the mutations and defaults from a schema?

  • Goodbye GraphiQL/network tab: you can't debug State link with usual tools since queries are not on the network. Instead you have to use the Chrome Apollo tools (which are fine).

  • Also, I am not sure if moving the apolloClient folder to client is a good idea. The problem is that then you have to wrap a lot of code into Meteor.isClient. Yet it does not make sense to create an apollo-client on the server (eg defining an InMemory cache), how was this handled this in the first place with the v1?

The default state is correctly set and retrieved, however I did not test the mutations yet.

I am trying to migrate an existing app to the new version. So far breaking changes are mostly related to RR4 (routes won't change for an obscure reason, and the API has changed, e.g currentRoute is not available anymore). The app seems ok with Apollo v2, since there are not much breaking changes. I guess SSR is also disabled?

@SachaG
Copy link
Contributor

SachaG commented Sep 28, 2018

Awesome, having a sample implementation of local state will be a big help. Like in the other PR I would probably try to avoid using Meteor.isClient but it's not a big deal.

The syntax with Apollo Link State is far more verbose than with Redux. It seems that the Apollo devs are waiting from more feedback from the community before they provide helpers. There are some redundancies between the gql queries and the mutations.
So I think it would be relevant to implement some helper functions, maybe smth similar to Redux or Rematch API, or even maybe to generate the mutations and defaults from a schema?

I would say let's keep it verbose for now and avoid adding too many abstraction layers, and then once we have enough practice using it we can "vulcanize" it with an intermediary layer like we do with React Router. Using a simplified state management API like Rematch for apollo-link-state would be great, in fact that could/should be something that exists outside of Vulcan I guess?

Also, I am not sure if moving the apolloClient folder to client is a good idea. The problem is that then you have to wrap a lot of code into Meteor.isClient. Yet it does not make sense to create an apollo-client on the server (eg defining an InMemory cache), how was this handled this in the first place with the v1?

We should ideally never use Meteor.isClient as other non-Meteor frameworks don't usually support mixing client/server code in the same file. So it'll just create headaches in the future if we rely on that pattern. And we do need to load some Apollo Client code on the server for rehydration and SSR, but I'm not super familiar with that part of the code yet.

The app seems ok with Apollo v2, since there are not much breaking changes. I guess SSR is also disabled?

Yes, I wanted to get the client part working first before worrying about SSR.

@SachaG
Copy link
Contributor

SachaG commented Sep 28, 2018

Btw feel free to merge the PR yourself, let's focus on making things work for now and we can refine everything later.

@eric-burel
Copy link
Contributor Author

Okay then we can move the apollo client back into modules if it works as expected, that was premature optimization. We can still write 2 different client/server implementation when things are clearer or if it becomes necessary.
I'll merge for now, I might be able to come back at it next week depending on my planning. There might be small bugs left in the Flash messages mutations since I could not test them yet but they will be easy to notice and fix.

For documentation purposes regarding your question on the previous PR:

  • registerDefault is equivalent to defining redux initialState
  • registerMutation is equivalent to defining a redux action and a corresponding reducer. Syntax is the same as a resolver (object, args, context). However you mutate the state (more close to vuex than redux here)
  • cache is equivalent to the redux state. It is available as context.cache in the mutations, cache.writeData allows to mutate it
  • the apollo client is thus the store
  • GraphQL queries with the @client directive are equivalent to redux selectors
  • graphql acts like mapDispatchToProps (for mutations) and mapStateToProps (for simple queries)
  • you have access to the full power of GraphQL, so you can mix client and server stuff, e.g if you want to retrieve both network and local data in the same query

@eric-burel eric-burel merged commit 90d0e0c into VulcanJS:apollo2 Sep 28, 2018
@SachaG
Copy link
Contributor

SachaG commented Sep 28, 2018

My only feedback is that registerDefault and registerMutation are a bit too generic and could mean anything… Maybe registerCacheDefault and registerCacheMutation? Or something that makes it clear that they apply to apollo-link-state? registerStateDefault? registerLinkStateDefault?

Since those won't be used that frequently I think we can err on the side of being more verbose and explicit.

@eric-burel
Copy link
Contributor Author

Definitely, I will think about it. state is a better fit than cache here, since a cache is usually something that is managed automatically contrary to application state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants