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

Initial mutations implementation #1

Merged
merged 3 commits into from
Feb 13, 2020
Merged

Initial mutations implementation #1

merged 3 commits into from
Feb 13, 2020

Conversation

dOrgJelli
Copy link
Collaborator

No description provided.

@dOrgJelli dOrgJelli requested a review from Jannis February 5, 2020 08:12
@Jannis Jannis changed the title Init Initial mutations implementation Feb 10, 2020
@Jannis
Copy link
Contributor

Jannis commented Feb 10, 2020

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

@dOrgJelli
Copy link
Collaborator Author

@Jannis is this better?

@dOrgJelli
Copy link
Collaborator Author

Also side note: master is not currently protected.

@Jannis
Copy link
Contributor

Jannis commented Feb 10, 2020

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

@dOrgJelli
Copy link
Collaborator Author

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

Copy link
Contributor

@Jannis Jannis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few general notes:

  • Let's drop the @client directive everywhere. Apollo warns if you use it wthout Apollo client-side resolvers.
  • The tests checking whether useMutation and <Mutation /> set the observer object seem superfluous.
  • The files in this repo are formatted inconsistently. Please run Prettier over all of them.
  • Error messages sometimes end with a . and sometimes they don't. Let's remove all .s.
  • Can we drop the _ on private / internal fields? Underscores are primarily used for indicating unused variables these days. Besides, they are not pretty. 😉

CONTRIBUTING.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
package.json Show resolved Hide resolved
packages/mutations-apollo-react/package.json Outdated Show resolved Hide resolved
packages/mutations/src/utils/array.ts Outdated Show resolved Hide resolved
packages/mutations/src/utils/async.ts Outdated Show resolved Hide resolved
packages/mutations/src/utils/async.ts Outdated Show resolved Hide resolved
packages/mutations/src/utils/schemaParsing.ts Outdated Show resolved Hide resolved
packages/mutations/src/utils/schemaParsing.ts Outdated Show resolved Hide resolved
@dOrgJelli
Copy link
Collaborator Author

dOrgJelli commented Feb 13, 2020

@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
Copy link
Contributor

Jannis commented Feb 13, 2020

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
Copy link
Contributor

Jannis commented Feb 13, 2020

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

  • The schema is passed to createMutations along with the resolvers. We're not using the schema anywhere right now, which, as I am realizing now, is wrong.
  • createMutations builds a GraphQLSchema object from the parsed schema and the resolvers.
  • localResolver goes away and is replaced with a simple call to execute.

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.

Comment on lines +31 to +35
cache.writeData({
data: {
getTodos: [],
},
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is needed for anything, so could be dropped.

@Jannis
Copy link
Contributor

Jannis commented Feb 13, 2020

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

@Jannis
Copy link
Contributor

Jannis commented Feb 13, 2020

Pushed a fix for Travis.

@Jannis
Copy link
Contributor

Jannis commented Feb 13, 2020

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.

@Jannis Jannis merged commit 5e50374 into master Feb 13, 2020
@dOrgJelli dOrgJelli deleted the init branch February 25, 2020 04:30
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.

None yet

3 participants