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

2.7.0 breaks existing typescript code #150

Closed
cerrochoclo opened this issue Feb 7, 2018 · 7 comments · Fixed by #285
Closed

2.7.0 breaks existing typescript code #150

cerrochoclo opened this issue Feb 7, 2018 · 7 comments · Fixed by #285

Comments

@cerrochoclo
Copy link

this code:

//file1.ts
import gql from 'graphql-tag';

export const someMutation = gql`someMutation`;
//file2.ts
import { ApolloClient } from 'apollo-client';
import {someMutation} from './file1.ts';

const apollo = new ApolloClient(...);
apollo.mutate({
      mutation: someMutation ,
      variables: {
//..
      }
    })

Those are the dependencies that work

@types\node@8.5.10
@types\node-fetch@1.6.7
apollo-cache-inmemory@1.1.7
apollo-client@2.2.2
apollo-link@1.0.7
apollo-link-context@1.0.3
apollo-link-http@1.3.2
graphql@0.11.7
graphql-tag@2.6.1
node-fetch@1.7.3
ts-node@4.1.0
typescript@2.7.1

but when updating to graphql-tag@2.7.0 we get and error:

error TS4023: Exported variable 'someMutation ' has or is using name 'DocumentNode' from external module "<my-project>node_modules/@types/graphql/language/ast" but cannot be named.
src/index.ts(38,3): error TS4053: Return type of public method from exported class has or is using name 'ExecutionResult' from external module "<my-project>/node_modules/@types/graphql/execution/execute" but cannot be named.

staying with 2.6.1 for now. Thanks!

@jnwng
Copy link
Contributor

jnwng commented Feb 7, 2018

/cc @felixfbecker in case you have an idea as to what's going on here — im not super familiar with typescript but i'll try and repro.

i'll revert this change in an hour if i can't fix it.

@jnwng
Copy link
Contributor

jnwng commented Feb 7, 2018

based on https://stackoverflow.com/questions/43335336/error-ts4058-return-type-of-exported-function-has-or-is-using-name-x-from-exter it looks like TypeScript users will have to explicitly import DocumentNode in their index.d.ts in order to use graphql-tag properly. @cerrochoclo can you confirm that that resolves at least the first error?

if this is the case, im going to revert this change since its a breaking API change or at least consider it in a major change

@jnwng
Copy link
Contributor

jnwng commented Feb 7, 2018

reverted in 2.7.3 pending further investigation. see #141 for some context as to why this was added in the first place, but i think this might be a breaking change

@felixfbecker
Copy link
Contributor

Yeah this is a known issue in TypeScript: microsoft/TypeScript#9944
The fix is to add an import { DocumentNode } from 'graphql' at the top of your file (maybe we should re-export that interface?). Not sure what you mean with index.d.ts though.

@jnwng
Copy link
Contributor

jnwng commented Feb 7, 2018

ah, i see that would be helpful to know what the workflow change is here. if we re-export the interface within graphql-tag, will it mean that existing users won't see any breaking changes?

if existing users need to change their code, then we need to mark this with a major version change

@felixfbecker
Copy link
Contributor

Tbh I don't know, I think it would still be required. But it would prevent another problem which is if you have multiple versions, there is no way for you to import the correct version of DocumentNode.

@cgatian
Copy link

cgatian commented Jun 12, 2018

I still wish this was a thing, even if it was breaking. 😄

jnwng added a commit that referenced this issue Oct 2, 2018
This reverts commit ae792b6. Based on #196 and #150, introducing the `DocumentNode` return value causes this library to break for existing TypeScript installations. I'm reverting this temporarily so we can release support for GraphQL v14 in a minor patch, and if we can't fix the TypeScript configuration, we'll release a major version with this commit back in.
jnwng added a commit that referenced this issue Oct 2, 2018
This reverts commit ae792b6. Based on #196 and #150, introducing the `DocumentNode` return value causes this library to break for existing TypeScript installations. I'm reverting this temporarily so we can release support for GraphQL v14 in a minor patch, and if we can't fix the TypeScript configuration, we'll release a major version with this commit back in.
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 a pull request may close this issue.

4 participants