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

Consider decoupling CRA 2.0 from graphql-tag version #4893

Closed
kassens opened this issue Aug 15, 2018 · 8 comments
Closed

Consider decoupling CRA 2.0 from graphql-tag version #4893

kassens opened this issue Aug 15, 2018 · 8 comments
Milestone

Comments

@kassens
Copy link
Member

kassens commented Aug 15, 2018

#3909 added support for importing *.graphql files. This is only useful for GraphQL libraries that want to consume the AST from the graphql package at the particular version bundled with CRA. This ties the upgrade of graphql and CRA together which seems limiting.

Relay (which doesn't import GraphQL, but instead uses inline GraphQL) solves this in the next version by supporting babel-plugin-macros which will look similar to:

import graphql from 'babel-plugin-relay/macro';
graphql`fragment Test on User { name }`

I'm not sure if loaders could be made 'injectable' in some way, but decoupling the projects might make upgrades easier going forward.

I want to add that keeping *.graphql require support specific to Apollo doesn't get in the way of Relay's solution.

@kassens
Copy link
Member Author

kassens commented Aug 15, 2018

cc @petetnt

@kassens kassens added this to the 2.0.0 milestone Aug 15, 2018
@kassens
Copy link
Member Author

kassens commented Aug 15, 2018

Adding 2.0.0 milestone as this decision should probably happen before the final release.

@petetnt
Copy link
Contributor

petetnt commented Aug 15, 2018

Hey!

While not directly supported by CRA, overriding the resolutions in package.json fields should enable changing the graphql-js (and the graphql-tag/loader version) independently from what CRA ships with if needed at least with yarn. That said it does limit the usage for npm users...

I haven't had much experience with Relay Classic or Relay Modern, but I was under assumption that both of them consume the same kind of JS object that the graphql-tag/loader produces? The PR (facebook/relay#2171) does enable the ahead of time compilation for CRA2.0 (which supports babel-plugin-macros) without baking the loader to the CRA (which is neat!) but wouldn't importing work for the Relay users too? There seems to be a graphql-tag.macro too, not sure if it's up-to-date.

That said I'd love to see optimal solution for this, what would be the optimal way forward? 👍

@kassens
Copy link
Member Author

kassens commented Aug 16, 2018

It seems to me that it'd be best if CRA wouldn't be coupled with specific libraries (except for React of course) and rely on a plugin systems such as babel-plugin-macros or for a custom loader maybe configured via #3917?

Thoughts @gaearon ?

@petetnt
Copy link
Contributor

petetnt commented Aug 16, 2018

Relevant: #4590 (comment)

I think the loader approach fits the CRA philosophy quite well: if the version doesn't fit, you can always eject but then you are on your own (which is totally fine, as it says in the box). I also think that vue-cli and angular-cli aren't orthogonal to cra in general.

The loader was vocally wanted in the past and people are pretty into it, but the caveats you've mentioned (coupling graphql version with CRA) hasn't been on the table so much but the same thing applies to things like svgr, or even the babel and webpack versions CRA@2 is using.

As Relay works too via the babel-plugin-macro, I think documenting the feature and the limitations would be sufficient. The Relay plugin is also very much appriciated so it's great to have despite the approach being different.

@iansu
Copy link
Contributor

iansu commented Aug 16, 2018

Is there something similar to graphql-tag/loader that's implemented using babel-plugin-macro? If so that might be a better way forward. That way the approach would be similar for Relay and Apollo and we wouldn't be tying people to a specific version of graphql-tag.

@petetnt
Copy link
Contributor

petetnt commented Aug 16, 2018

https://github.com/leoasis/graphql-tag.macro should offer similar functionality.

@Timer
Copy link
Contributor

Timer commented Sep 24, 2018

Was the general consensus here that we revert #3909?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants