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

feat(deps): Remove Printer and Parser re-exports #50

Merged
merged 1 commit into from
Mar 28, 2017
Merged

Conversation

abhiaiyer91
Copy link
Contributor

@abhiaiyer91 abhiaiyer91 commented Feb 8, 2017

#49

@stubailo
Copy link
Contributor

stubailo commented Feb 8, 2017

I think we may as well export "parse" right? Since this package depends on it anyway?

@abhiaiyer91
Copy link
Contributor Author

Well I was thinking if we remove both and just make GraphQL a peer dependency we don't have to struggle with mantaining a version of GraphQL as a dependency in the repo and just have a peer dep.

Imho I think its better to have the use the functions directly from GraphQL

@helfer
Copy link
Contributor

helfer commented Feb 10, 2017

I'll defer to @jnwng on this one. What would your preference be?

@jnwng
Copy link
Contributor

jnwng commented Feb 10, 2017

i agree with @abhiaiyer91 - if we're not doing anything special with print or parse and just re-exporting it, seems better to have people use it upstream from graphql instead (presumably safe given that graphql is now a peer dependency).

however, this is a breaking API change for anyone using either of these exported functions, and i would suggest deprecating them in a minor / patch version (1.3 or 1.2.x) and perhaps removing it entirely when we think we need to cut a new major version. this may seem a little drastic, but is a little more honest to how we're versioning the package, and at least gives people who are using graphql-tag@^1.x.x some heads up that something changed.

that being said, its very likely the case that a new major version is not needed or is just not even in the works, so it might be a bit until it makes sense to remove this. does the inclusion of graphql/language/print make it to production clients? if so, we may want to do a new major/minor version sooner rather than later. if not, sounds like it could wait.

@jnwng
Copy link
Contributor

jnwng commented Feb 10, 2017

its also worth noting that apollographql/apollo-client depends on this export in a few places, so we may need to be a bit more careful about versioning this change.

EDIT: angular-apollo and react-apollo also depend on this export

@helfer
Copy link
Contributor

helfer commented Feb 10, 2017

@jnwng yeah, that's right. I think the first thing we should make sure is that print can be imported from graphql-js without importing a whole bunch of other stuff (and thus increasing the bundle size). @abhiaiyer91 can you make PRs to apollo-client, react-apollo and angular-apollo that will do that? I want to first make sure the change to those repos looks good before moving ahead with this one.

@stubailo
Copy link
Contributor

Yeah I think we definitely need to bump the major version of graphql-tag if we make this very significant change.

@jnwng jnwng merged commit a312c02 into master Mar 28, 2017
@jnwng
Copy link
Contributor

jnwng commented Mar 28, 2017

thanks @abhiaiyer91!

@jnwng jnwng deleted the StopBundling branch March 28, 2017 16:43
jnwng pushed a commit to snaptech/graphql-tag that referenced this pull request May 11, 2017
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.

4 participants