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

[RFC] Fragment imports and file format #343

Closed
tgvashworth opened this issue Aug 15, 2017 · 22 comments
Closed

[RFC] Fragment imports and file format #343

tgvashworth opened this issue Aug 15, 2017 · 22 comments
Labels
🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)

Comments

@tgvashworth
Copy link

tgvashworth commented Aug 15, 2017

Hi GraphQLers,

I'd like feedback on an idea to spec the .graphql file format and, within that, to spec a syntax for "importing" fragments from other files (inspired by apollographql/graphql-tag#33 by @Poincare and @stubailo).

I would like to see this specced so developers can store their application's queries in files, co-located with the associated application code, that can be consumed by tooling in multiple languages & platforms.


A little aside... It's particularly useful for us because our migration strategy (from REST to GraphQL) is to replace existing network requests with GraphQL queries, and we use a stored operation/persisted query system, so the queries have to be built and submitted before reaching prod. The queries should be stored with the code, in a way that can be statically built and uploaded with a standalone tool.

Specifically, I want to avoid:

  • requiring developers to maintain massive files that contain single queries
  • duplication of identical fragments within those queries (for standard objects that never change across the API)

In my mind, the file format spec is relatively simple, requiring:

  • extension (.graphql?)
  • character encoding (Unicode? I'm not a specialist here; unsure of nuance)
  • MIME type? (text/graphql?)

To avoid fragment duplication, I would also like to spec the import syntax for fragments. The goals of this syntax are that:

  • large query files can be split into fragments and the composite query
  • .graphql files are transferable between platforms (eg. written for Android but parsed by Scala)
  • be completely opt-in and not break existing GraphQL files

Feature-wise, imports would:

  • support direct import: fragments can be imported from another file
  • support transitive import: dependencies of a dependency are imported too
  • not support circular imports
  • only support relative-path imports (no URLs or absolute paths)

Fragments would be imported in their entirety — you cannot import by name and you cannot alias. This is to simplify implementation and to dodge the namespacing problem (YAGNI). Put another way, avoiding fragment name clashes is a user-land problem.

Regarding circular imports, they should be considered invalid: if fragment file "a" imports fragment file "b", which imports "a" — the latter "a" would be invalid and should cause an error in the implementation.

In terms of syntax, there are some more goals:

  • look GraphQL-ish
  • support future extension if named imports proves useful & achievable

I suggest, inspired by JavaScript and @poincare's work:

# file: my_user.graphql

import "./basic_user.graphql"

query ($id: ID) {
  user(id: $id) {
     ... BasicUser
     my_field
  }
}
# file: basic_user.graphql

fragment BasicUser on User {
  id
  name
}

Thoughts?

Initially, please give me a sense of:

  • something you need: yes/no?
  • something you can forsee needing? yes/no?
  • is there a viable alternative to this given the goals above? (YAGNI!)
  • is there more prior art here? I have also see it mentioned here.

Please let's avoid bikeshedding on the import syntax; that can come later. 🚲

@brokenpeace
Copy link

brokenpeace commented Aug 15, 2017 via email

@tgvashworth
Copy link
Author

@brokenpeace Don't worry, this idea is opt in (as are persisted queries). You can keep using GraphQL the way you are! ☺️

@brokenpeace
Copy link

brokenpeace commented Aug 15, 2017

How about ability as second part mentioned utilizing both? If it does provide benefit for a lot of use cases just my few particular ones. I do like the concept.

Actually looking at the esparse library we can just add optionals in this case perhaps we use the template name+hash of the fields in a cached persisted folder? 🔨

I dig the idea completely but I am just trying to figure out how to keep it simple yet flexible (which is what I love about it)

@robrichard
Copy link
Contributor

It would be great to see some form of this incorporated within relay-compiler. Seems like a great way avoid issues with more than one file containing fragments with the same name, e.g. a/Foo.js and b/Foo.js both contain a Foo_viewer fragment). See facebook/relay#2020

@stubailo
Copy link
Contributor

Yep - one reason we didn't go with the Relay Modern approach for fragments in Apollo is to avoid having a global namespace where you could run into naming conflicts. It would be nice to have a module-like approach.

One cute thing suggested by @calebmer once was:

query {
  me {
    firstName
    ...userDetails @import(from: "../UserDetails")
  }
}

I think all tooling should be built to work equally well with queries in JS code and queries in standalone files. For example, both Apollo and Relay persisted queries work fine with queries written as part of JS code (but require unique fragment names).

@tgvashworth
Copy link
Author

tgvashworth commented Aug 16, 2017

@stubailo Do you have specific feedback on the proposal? It seems you're saying that you don't like that fragments would exist in a global namespace?

Avoiding the global import adds a lot of complexity to the spec & tooling that we might not want to incur:

  • how are fragments namespaced — do you import them by name? can you alias?
  • if you import a fragment by name, what happens to its dependencies?
  • what if a transitive dependency has a name clash? do you auto-namespace transitive dependencies?

I'm keen to get a simple version specced soon, as the community is already running into this problem and solving it ad-hoc. We can leave the door open to improvements like namespacing in the future.

The way Apollo does persisted queries (which I haven't used, so my understanding is based on this) seems to be compatible with this proposal.

@stubailo
Copy link
Contributor

Here's my current set of opinions:

  1. Having imports is valuable
  2. I really like the ES2015 module system - stuff like circular dependencies and being able to import just one fragment from a document is really useful (like having a fragments.graphql and importing individual fragments from there seems like a pretty good pattern)
  3. We should definitely follow dependencies, this should be simple because GraphQL as a language is quite simple
  4. Things that break the GraphQL parser/AST are harder to test out quickly, so a short-term solution based on directives or comments is desirable

Regarding circular imports, they should be considered invalid

I don't think this should be the case since in GraphQL it should be pretty easy to resolve any dependencies like that.

So basically:

# PersonFragments.graphql
fragment PersonBasicInfo on Person {
  firstName
  lastName
  avatar { small }
}

fragment PersonDetails on Person {
  ...PersonBasicInfo
  address { street, state, zipCode }
}

Then you can import:

import { PersonDetails } from "./PersonFragments";

query CurrentUserDetails {
  currentUser {
    ...PersonDetails
  }
}

Maybe you can rename to avoid name collisions:

import { PersonDetails } from "./PersonFragments";
import { PersonDetails as AvatarPersonDetails } from "./AvatarFragments";

query CurrentUserDetails {
  currentUser {
    ...PersonDetails
    ...AvatarPersonDetails
  }
}

So the name collision thing is interesting. One situation I'm worried about is where you have two parts of your app you used to use separately, but then you want to use a fragment across that boundary. You might have a whole world of duplicate fragment names and renaming them all is not feasible. That seems to imply that it would be valuable to have some way to import two fragments with the same name and have it still work.

However, there's no scoping in GraphQL - all fragment names are totally global. So either we need to assign that fragment a unique name as part of the import evaluation (not ideal) or convert it to an inline fragment (also not ideal).

I'm keen to get a simple version specced soon, as the community is already running into this problem and solving it ad-hoc. We can leave the door open to improvements like namespacing in the future.

Is there a disadvantage to using the current experimental approach with comments until a good spec is finalized? Something like a module system probably shouldn't be added lightly.

@tgvashworth
Copy link
Author

I don't think this should be the case since in GraphQL it should be pretty easy to resolve any dependencies like that.

Maybe. I don't think circular dependencies are that simple. It's certainly more simple than in an imperative language, but you still have to worry about ordering (do you hoist the import statements? if not, what are the implications?). I need to think more about edge cases of circular imports.

So either we need to assign that fragment a unique name as part of the import evaluation (not ideal) or convert it to an inline fragment (also not ideal).

This is the transitive problem I mentioned. As soon as imports become more advanced than a literal file-contents copy, it become an issue that needs specification. That makes the spec more complex, which is generally undesirable.

It's preferable in this case to see how developers solve that problem (automatic renaming? failing the build?) and, perhaps, later encode it in the spec.

Is there a disadvantage to using the current experimental approach with comments until a good spec is finalized? Something like a module system probably shouldn't be added lightly.

No, it's fine, but implementations will diverge and may become incompatible. It will be painful to walk back from that for everyone.

A little bit of experimentation in the community is great, and is what inspired filing this issue!

@stubailo
Copy link
Contributor

but you still have to worry about ordering

That's one nice thing - ordering doesn't matter in GraphQL, so I think the issue becomes much simpler.

As soon as imports become more advanced than a literal file-contents copy

So the current comment-based approach, as you've seen, is a straight copy. I think if we decide that approach is fine, it doesn't need to be in the language spec IMO. Then it's more like a preprocessor?

I guess I'm not clear on how preprocessor stuff works in C, for example - is it part of the language spec?

@rmosolgo
Copy link

Just wanted to say 👍 on experimentation for the time being. I just recently learned graphql-tag's approach which is great, also, github/graphql-client uses Ruby constants to import GraphQL fragments. Kind of crazy, but just thought I'd share another example 😆

@tgvashworth
Copy link
Author

I think if we decide that approach is fine, it doesn't need to be in the language spec IMO.

I think it's worth having something specced so you can be reasonably sure you have compatible implementations. Perhaps if not in this spec, it would be worth writing down the rules somewhere.


I thought in more depth about a more complex import syntax, supporting import-by-name and aliasing, and I think you're right: it's useful enough that it might be worth talking through in detail.

If the features were:

  • support direct import: fragments are imported from another file by name
  • support aliasing: fragments can be renamed during import
  • support transitive import: dependencies of a dependency are imported too
  • circular imports allowed, with cycle detection
  • only support relative-path imports (no URLs or absolute paths)

Then we could look at the syntax you describe (taken from JS):

import { User as MyUser } from "./user"

However, before we get there I want to discuss: is there a way to achieve the goals stated above without introducing new syntax? If we can then it's the preferable option, I think.

@stubailo
Copy link
Contributor

I think a directive-based approach would work to satisfy all of the above cases, without new syntax:

query {
  me {
    firstName
    ...userDetails @import(name: "specificUserDetails", from: "../UserDetails")
  }
}

@tgvashworth
Copy link
Author

Without getting too much into syntax, I'm really keen to address the question at the bottom of my comment:

is there a way to achieve the goals stated above without introducing new syntax?

Are there any alternatives?

@wincent
Copy link
Contributor

wincent commented Aug 23, 2017

Are there any alternatives?

FWIW, Facebook has gotten pretty far without having imports, even building (almost certainly) the largest and (definitely) the oldest GraphQL-powered apps in the world. Query and fragment names are "globally" unique (within each project), and it is quite easy to jump from the place where a fragment is spread to where it is defined thanks to tooling like graphql-language-service. Furthermore, our experience building the Relay compiler has shown us that it is possible to build an optimizing pipeline that eliminates redundancy, and our use of persisted queries mean that query size doesn't matter (in terms of client-to-server network transport, the cost is basically fixed; and in the other direction you pay only for what you use, so it's best to make sure you're declaring what you use as close as possible to the use site so as to avoid accidental overfetching).

This is not to suggest that the idea is a non-starter: just that the value isn't necessarily obvious compared to a world without imports.

@stubailo
Copy link
Contributor

I do think this is one case though where the existence of a system where imports weren't used doesn't mean they are never going to be valuable. Sure it's possible to get by without them, but it's also possible to use programming languages without any module system.

I think imports are valuable to:

  1. Integrate with module-based systems like JavaScript frontends
  2. Avoid the necessity for globally unique fragment names, which can become a hassle if you develop two code bases in isolation and then need to combine them into one app
  3. Identify where fragments are coming from in a large codebase without specialized tooling

@wincent
Copy link
Contributor

wincent commented Aug 23, 2017

I do think this is one case though where the existence of a system where imports weren't used doesn't mean they are never going to be valuable.

I certainly didn't make that claim. Where did you see me make it? I was just offering a (significant, I would think) data point. What I said was (with emphasis added):

This is not to suggest that the idea is a non-starter: just that the value isn't necessarily obvious compared to a world without imports.

Your list of benefits is good. But the proposal is not without costs. One example: any system that moves away from the current global system towards one involving relative paths means that you won't be able to move a heavily-depended upon fragment without modifying all the import sites. There are probably others.

Once again, just to be clear: I'm not trying to shut the discussion down, I'm just trying to offer a counterpoint to make sure that we're considering as many sides of the issue as possible.

@stubailo
Copy link
Contributor

I certainly didn't make that claim. Where did you see me make it?

I didn't say you made it, sorry if it seemed that way!

@robrichard
Copy link
Contributor

I can understand how it's easy to get by without imports when the following restrictions are in place (which to the best of my understanding is the case at Facebook).

It's much more awkward when you try to adopt it to an existing code base with a nested directory structure, non-unique file names, and fragments in modules that are shared via a packaging system like npm.

@robrichard
Copy link
Contributor

I've been working on a proof of concept of @stubailo's directive based proposal in relay-compiler: facebook/relay#2064

@tgvashworth
Copy link
Author

@wincent Thanks for your input here. I have some follow-up questions...

How are full queries constructed at Facebook? From what I gather you have a collection of (named?) queries and fragments. Looking at Relay, they're written in code directly (not files) — is this also true for your native clients?

Do you have equivalents to the Relay compiler for Android and iOS queries?

@wincent
Copy link
Contributor

wincent commented Aug 31, 2017

How are full queries constructed at Facebook? From what I gather you have a collection of (named?) queries and fragments. Looking at Relay, they're written in code directly (not files) — is this also true for your native clients?

Do you have equivalents to the Relay compiler for Android and iOS queries?

For native codebases, developers write GraphQL in .graphql files. We do have an equivalent to the Relay compiler that builds native artifacts (persisted queries, fragment/type model classes, query "builders" etc). We configure projects using .graphqlconfig files. And use graphql-language-service to provide code intelligence (autocompletion, diagnostics, "go to definition" etc). Going into further detail is probably beyond the scope of this issue, though; the nutshell version is that it is basically analogous to how Relay works.

@leebyron
Copy link
Collaborator

leebyron commented Oct 2, 2018

Closing this aging RFC. There are a few competing import proposals and the discussion hear led to some compelling reasons why different tools would want their own.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🗑 Rejected (RFC X) RFC Stage X (See CONTRIBUTING.md)
Projects
None yet
Development

No branches or pull requests

7 participants