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

Fixed type definition for subscribe #1290

Merged

Conversation

intellix
Copy link
Contributor

@intellix intellix commented Aug 4, 2019

Checklist:

  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests
  • Try to include the Pull Request inside of CHANGELOG.md

According to apollo-client it returns a FetchResult whereas apollo-angular says it's an any: https://github.com/apollographql/apollo-client/blob/7eaf4132cd2cd6244260777799406aaa03fcf377/packages/apollo-client/src/ApolloClient.ts#L355

@intellix intellix force-pushed the fix-subscribe-return-type branch from 1f65e95 to 3520a64 Compare August 4, 2019 11:23
@intellix
Copy link
Contributor Author

intellix commented Aug 9, 2019

@kamilkisiela any idea why the above 2 checks aren't finishing after days?

@kamilkisiela
Copy link
Owner

@intellix could you rebase?

@intellix intellix force-pushed the fix-subscribe-return-type branch from 3520a64 to 017d6d8 Compare September 3, 2019 15:55
@intellix
Copy link
Contributor Author

intellix commented Sep 3, 2019

Rebased @kamilkisiela

@kamilkisiela
Copy link
Owner

Thank you @intellix !! 🚀 🥇

@kamilkisiela kamilkisiela merged commit 2594eb4 into kamilkisiela:master Sep 3, 2019
@intellix intellix deleted the fix-subscribe-return-type branch September 3, 2019 16:02
@kamilkisiela kamilkisiela mentioned this pull request Sep 3, 2019
@kamilkisiela
Copy link
Owner

v1.7.0

@PowerKiKi
Copy link
Collaborator

I think this is a breaking change that should have been release as a major version. It would typically break the following code:

export interface Login {
  login: boolean;
}

this.apollo.mutate<Login>({
    mutation: loginMutation,
    variables: loginData,
}).pipe(map(({data: {login}}) => login));

With an error similar to:

error TS2339: Property 'login' does not exist on type 'Login | null | undefined'.

This is because ultimately ExecutionResult is defined with a nullable data property in graphql package. So all existing mutations must be adapted accordingly.

I am not sure why data is nullable though, I believe in the context of apollo-angular (not graphql), a mutation either succeeds and its data is exactly as defined in the mutation itself (probably often non-nullable), or the mutation fails and it goes through the observable error mechanism. But we should never force a mutation result to be nullable... or should we ?

@PowerKiKi
Copy link
Collaborator

@intellix @kamilkisiela do you have any thoughts on my comment ?

As is it is pretty much impossible to use strictNullChecks in combinaison with this PR.

PowerKiKi added a commit to Ecodev/artisans that referenced this pull request Dec 7, 2019
@PowerKiKi
Copy link
Collaborator

I digged deeper in this issue and finally figured out the issue is in apollo-client. For details see apollographql/apollo-client#5662

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.

3 participants