parcel-bundler / parcel

The zero configuration build tool for the web. 📦🚀
https://parceljs.org
MIT License
43.37k stars 2.26k forks source link

Support named imports in graphql transformer #4472

Closed squirly closed 4 years ago

squirly commented 4 years ago

🙋 feature request

Some graphql tooling supports named imports. See examples below.

🤔 Expected Behavior

#import MyFragment1, MyFragment2 from './common.graphql'

query MyQuery {
  stuff {
    ...MyFragment1
    ...MyFragment2
  }
}

😯 Current Behavior

The import is ignored.

💁 Possible Solution

Use the plugable Document Loading from @graphql-toolkit/core. This supports custom file loaders that can interop with the Parcel loading logic.

This will require reworking the current implementation to do full AST parsing and combining of the GraphQL.

The AST would need to then be printed out and could also be minified, see stripIgnoredCharacters.

🔦 Context

I am using shared fragment files in various graphQL files. Other tooling that I am using does not allow unused fragments in a document.

💻 Examples

See https://github.com/ardatan/graphql-import#usage

mischnic commented 4 years ago

That would be a transformer that runs before the existings @parcel/transformer-graphql (which should in that case be renamed to something like @parcel/transformer-graphql-js) and essentially inlines the import (if I understand that feature correctly).

squirly commented 4 years ago

Interesting. I would be happy to make these changes.

Have a couple of questions before I get started:

mischnic commented 4 years ago

What are the reasons why this cannot be a part of the existing @parcel/transformer-graphql? It should be relatively easy to make it backwards compatible, and adds onto the existing import implementation.

Is this a standard graphql syntax? Then I'd say it's fine in @parcel/transformer-graphql

If not, maybe someone wants to use a different library with a different behaviour and the same syntax. In that case they would have to duplicate some of the code in @parcel/transformer-graphql (the gql to js part)

Does this pose any issues with how Parcel transforms files?

I didn't quite understand what you are referring to. But generally there are two mechanism in Parcel for "bundling":

  1. a transformer just modifies the one asset and leaves the merging to a packager (done with JS, CSS, HTML
  2. in case (1) is overkill (I would say this is the case here) or not possible (e.g. with the Less compiler or Pug or PostCSS plugins that need access to imported files) you can do everything in the transformer and tell Parcel via addIncludedFile that there are some files that Parcel doesn't yet know about and should be watched.

Does this answer your question?

Should this rename or be a new package in Parcel?

A new package (or not, see previous point). The existing package is called @parcel/transformer-graphql but that doesn't convey the meaning "to js", especially if we have multiple graphql-related packages.

What kind of logic should be applied for minification? Is there some configuration that indicates the transformer should minify?

If there are some specific options to enable optimizations, these can be applied based on options.minify or options.mode. So a Rust transformer might want to change the compiler optimization levels. Terser is run after the bundles are generated, so that it can do cross-asset optimizations.

squirly commented 4 years ago

Thanks for the excellent clarifications!

Sitting on this overnight I think I have a clearer proposal:

Let me know if this makes sense and I can get started on this.

mischnic commented 4 years ago

I think we should just put everything into @parcel/transformer-graphql for now (unless there are two very popular but completely incompatible syntaxes). The import part can always be refactored out afterwards.

based on the options.minify or options.mode.

I think minify would make the most sense here.

I can get started on this.

👍

squirly commented 4 years ago

Further clarifications in: https://spectrum.chat/parcel/contributing/differentiation-of-assets-and-transformers~7d7deb5b-3d17-4d35-986d-d9b1a9c14149

wbinnssmith commented 4 years ago

Is this a part of the official GraphQL specification? If not, I'm not sure this should belong in the transformer maintained by Parcel.

DeMoorJasper commented 4 years ago

@wbinnssmith imports are not part of the graphql spec, but we did already support that in Parcel 1 so it seems fair to continue to support it in Parcel 2. Without imports graphql would be just a txt file and an inline transformer or fs.readfileSync would probably be the same.