graphprotocol / mutations

Apache License 2.0
4 stars 5 forks source link

Initial mutations implementation #1

Closed dOrgJelli closed 4 years ago

Jannis commented 4 years ago

@dOrgJelli Could we squash all of these commits into a single one, with a decent commit message?

dOrgJelli commented 4 years ago

@Jannis is this better?

dOrgJelli commented 4 years ago

Also side note: master is not currently protected.

Jannis commented 4 years ago

@dOrgJelli Now the commit message is too long 😉 How about "Initial implementation with Apollo support"?

dOrgJelli commented 4 years ago

Sure thing, will update shortly 😊 hopping on a flight to Denver ✈️ 🏔

dOrgJelli commented 4 years ago

@Jannis

Let's drop the @client directive everywhere. Apollo warns if you use it wthout Apollo client-side resolvers.

From what we've found, when executing the query using ApolloClient, the @client directive is required. If it isn't the resolver will not be found. We thought about adding @client behind the scenes inside of the localResolverExecutor, but the document's directives are const/uneditable.

The tests checking whether useMutation and set the observer object seem superfluous.

I'd like to keep these as it's the only e2e verification we have for the state observer.

The files in this repo are formatted inconsistently. Please run Prettier over all of them.

Fixed :)

Can we drop the _ on private / internal fields? Underscores are primarily used for indicating unused variables these days. Besides, they are not pretty.

Fixed :)

Jannis commented 4 years ago

From what we've found, when executing the query using ApolloClient, the @client directive is required. If it isn't the resolver will not be found. We thought about adding @client behind the scenes inside of the localResolverExecutor, but the document's directives are const/uneditable.

That's because in the mutations package, you use the Apollo-specific withClientState. That's not going to work, as the mutations package is to be agnostic of any GraphQL framework. There is really no need for ApolloLink there.

Jannis commented 4 years ago

What really needs to happen for mutations to be agnostic of Apollo is:

It is clear that this is too late for ETHDenver, so we're going to have to accept the @client directive for now. This is going to be an alpha release after all and we can still make breaking changes.

Jannis commented 4 years ago

@dOrgJelli @namesty Something's up with Travis, I reckon the TypeScript config for CI is broken a little.

Jannis commented 4 years ago

Pushed a fix for Travis.

Jannis commented 4 years ago

I'm going to go ahead and merge it. CI will pass, apart from the book deployment for the docs, which are empty right now anyway.