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

Revert "Add DocumentNode to gql declaration (#196)" #218

Merged
merged 1 commit into from
Oct 2, 2018

Conversation

jnwng
Copy link
Contributor

@jnwng jnwng commented 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.

/cc @felixfbecker @kamilkisiela since i still can't figure out how to fix the TypeScript installation in a non-breaking way.

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 jnwng merged commit c5a32a6 into master Oct 2, 2018
@jnwng jnwng deleted the jw/revert-documentnode branch October 2, 2018 05:36
@felixfbecker
Copy link
Contributor

In #150 I suggested to reexport the DocumentNode interface. I still believe that would prevent the error

@kamilkisiela
Copy link
Contributor

@jnwng I still don't think it's true what you said. TypeScript doesn't break anything if DocumentNode is correctly defined as a return value of a function.

In latests versions of TS it transforms each imported interface to something like this if they were implicit:

export default function gql(literals: any, ...placeholders: any[]): import('graphql/something/document-node').DocumentNode;

But we explicitly import DocumentNode and define it as a return type.

@kamilkisiela
Copy link
Contributor

@kamilkisiela/[email protected] is my copy of [email protected] with DocumentNode included

@felixfbecker
Copy link
Contributor

Frankly it baffles me that this same change was merged twice and reverted twice, instead of trying the fix I suggested.

@kamilkisiela
Copy link
Contributor

@jnwng @felixfbecker I would say, let's release it as a RC, make people check it and release a stable one. People scream about a lot of things but usually they use TypeScript incorrectly or doesn't understand it at all and half of the time it's their internal setup that's causing an issue.

@jnwng
Copy link
Contributor Author

jnwng commented Oct 2, 2018 via email

@Wintus Wintus mentioned this pull request Sep 25, 2019
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