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

Webpack: support multiple query definitions per query file #122

Merged
merged 6 commits into from
Dec 6, 2017

Conversation

jfaust
Copy link
Contributor

@jfaust jfaust commented Sep 8, 2017

This allows them to be imported by name:

// query.gql
query MyQuery1 {
  ...
}

query MyQuery2 {
  ...
}

// some.js
import { MyQuery1, MyQuery2 } from 'query.gql'

// or
const Queries = require('query.gql');
...Queries.MyQuery1
...Queries.MyQuery2

In the case where there's only a single query, it remains backwards compatible. I'm not sure if this is desired or not... I considered putting it behind a loader option, but thought I'd ask here.

I believe this solves #119, but the wording in that issue is unclear to me so I'm not sure.

@apollo-cla
Copy link

@jfaust: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@jfaust
Copy link
Contributor Author

jfaust commented Sep 8, 2017

Looking at the failing tests now...

@jnwng
Copy link
Contributor

jnwng commented Sep 8, 2017

@jfaust this looks awesome. do you mind adding tests for the new functionality and make sure we haven't regressed in behavior?

@stubailo
Copy link
Contributor

stubailo commented Sep 8, 2017

One concern I have here is the interaction with fragment imports. It wouldn't be possible to identify which fragments need to be included based on which queries are imported with the current setup.

@jfaust
Copy link
Contributor Author

jfaust commented Sep 8, 2017

@jnwng Yes, for sure. I'm confused as to why the current ones are failing though, as I was expecting this to be entirely backwards compatible. But I also forgot to run them locally, so I'm looking at those now.

@stubailo It works for fragments specified inside a query, but now that I think about it, nested dependencies will likely break. Is that what you mean? Something like:

// Frag1.gql
fragment Frag1 {
}

// Frag2.gql
#import './Fragment1.gql'
fragment Frag2 {
  ...Frag1
}

// Queries.gql
#import './Frag2.gql'

query Foo {
  ...Frag2
}

...

The only way I can see fixing that right now is by pushing the dependency collection into load time, which is should work I was just trying to do as much computation as possible at pack time.

@jfaust
Copy link
Contributor Author

jfaust commented Sep 8, 2017

It turned out a bit messier than I hoped, but I believe the latest commit solves and tests the nested fragment dependency issues.

… in them.

This is not really possible to test in the curren test framework, because it requires the load-time require()
@Cito
Copy link

Cito commented Oct 20, 2017

Works nicely for me. Can we get this into the next release?

With this solution, you can put related queries in one file, and shared fragments in the same file. You then don't even need the separate import mechanism for fragments.

@jamiter
Copy link
Contributor

jamiter commented Nov 2, 2017

Hi all! Is there something blocking this? What can we do to get this merged? Cheers.

@jfaust
Copy link
Contributor Author

jfaust commented Nov 2, 2017

We've been using it successfully for a while. Any concerns @jnwng @stubailo ?

@Cito
Copy link

Cito commented Dec 2, 2017

I'd really like to have this feature.

One concern might be that the behavior now is different depending on whether you have a single query or multiple ones. In the former case, it gets exported as the default value, so you must import directly, while in the latter case you must import by name. Now if you use the wrong kind of import statement, or refactor the graphql files, changing them from multiple to single or vice versa, and forget to adapt the corresponding import statement accordingly, the imported queries will be undefined. This has bitten me already several times, particularly since IDEs do not understand these GraphQL imports and do not warn when something is wrong. Maybe someone has an idea how to solve this and make it less error prone?

@lionkeng
Copy link

lionkeng commented Dec 6, 2017

Liked to see this merged as well if there are no block issues.

@jfaust
Copy link
Contributor Author

jfaust commented Dec 6, 2017

@Cito I considered putting it behind a webpack flag to enable -- so if you enabled it you'd always get named imports, and if not it would fall back to the previous behavior. I don't have a ton of time to work on it atm but can look into that at some point if it's considered worthwhile.

@jnwng jnwng merged commit ac4f0da into apollographql:master Dec 6, 2017
@jnwng
Copy link
Contributor

jnwng commented Dec 6, 2017

@Cito @jfaust i think preserving the fallback behavior will get this in sooner rather than later — i would expect that the natural evolution of this might be exporting both the default / named exports in the case of only having one query / operation.

thanks @jfaust for the hard work!

@jnwng
Copy link
Contributor

jnwng commented Dec 6, 2017

this is live in graphql-tag@2.6.0

@stubailo
Copy link
Contributor

stubailo commented Dec 6, 2017

yay!

@Cito
Copy link

Cito commented Dec 6, 2017

Thank you. Just tested it, works great - I can now use 2.6.0 without any patches!

Still, maybe it would be a good idea to export as default only for a single, unnamed query, or always export by name. Since this would break backward compatibility, we could make it configurable with a webpack flag as @jfaust suggested, and make this the default behavior in 3.0. Does this make sense? Shall I create a new ticket for this?

@JounQin
Copy link

JounQin commented Dec 20, 2017

Hey, guys. I have a tiny problem.

# query.gql case1: single with name
query MyQuery1 {
  ...
}

why it equals

# query.gql case2: single without name
query {
  ...
}

Should it be consistently?

import { MyQuery1 } from './query.gql' // case1
import query from './query.gql' // case2

But actually case1 outputs:

import query from './query.gql' // case1

@jnwng
Copy link
Contributor

jnwng commented Dec 20, 2017

@JounQin this is the problem that @Cito brought up. @Cito do you mind filing an issue? i think there's enough here to group into a breaking release of graphql-tag

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.

8 participants