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

Add loader for .graphql files #3909

Merged
merged 17 commits into from
Feb 3, 2018
Merged

Add loader for .graphql files #3909

merged 17 commits into from
Feb 3, 2018

Conversation

petetnt
Copy link
Contributor

@petetnt petetnt commented Jan 23, 2018

Important notice

The implicit loader was removed in favor of the macro approach, which gives much more flexibility (for example using different graphql-js version instead of the one shipped with CRA) with little to no code changes.

You can load .gql and .graphql files by installing the graphql-tag.macro from npm and using it like this:

import { loader } from 'graphql.macro';
const A = loader('./assets/graphql.graphql');

The loader statement gets replaced with the actual graphql-tag object on build time and the precomputed object also lowers performance overhead by bit, so it behaves like the loader in this PR did!

OLD PR

This PR adds the .graphql file loading feature discussed in #3873.

A POC of the concept can be found in https://github.com/petetnt/cra-pr-3909-poc/ (might need to link to react-scripts after installing). Tested the following things:

  • ✅ works while developing with yarn start
  • ✅ works after building with yarn build

Also looked at the output after ejecting and it looks correct, couldn't test it yet though after yarn eject due to some version issues 🤷

The PR adds following things:

  • Ability to load and preprocess .graphql and .gql files with graphql-tag/loader
  • Updates the README.md and User guide with descriptions and usage
  • Adds a jest transform so the .graphql files work with tests too

Tested it out with react-apollo and it worked beautifully. Should work with Relay Modern too, but prerequisite for it is #2343

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@iansu
Copy link
Contributor

iansu commented Jan 23, 2018

Nice! On first glance this looks good to me. Can you add an e2e test (in packages/react-scripts/fixtures/kitchensink/src/features/webpack) for importing a .graphql file?

@petetnt
Copy link
Contributor Author

petetnt commented Jan 23, 2018

Yep! Working on the tests right now!

@@ -44,9 +44,12 @@
"extract-text-webpack-plugin": "3.0.2",
"file-loader": "1.1.6",
"fs-extra": "5.0.0",
"graphql": "^0.12.3",
"graphql-tag": "^2.6.1",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to pin these new dependencies

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinned in d1dd2a8

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@petetnt
Copy link
Contributor Author

petetnt commented Jan 23, 2018

Added tests, running local tests with docker fail on the npm cache clean step, if the tests fail on the CI I'll investigate / fix that and fix the tests again 🤔

@iansu
Copy link
Contributor

iansu commented Jan 23, 2018

I don't think the local e2e tests in docker are working properly right now. Let's just see what happens on CI.

@iansu iansu added this to the 2.0.0 milestone Jan 23, 2018
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@petetnt
Copy link
Contributor Author

petetnt commented Jan 24, 2018

Tests now pass with 435ece0 on Travis, there's something weird going on at AppVeyor at large.

@@ -35,7 +35,10 @@ module.exports = (resolve, rootDir, isEjecting) => {
? '<rootDir>/node_modules/babel-jest'
: resolve('config/jest/babelTransform.js'),
'^.+\\.css$': resolve('config/jest/cssTransform.js'),
'^(?!.*\\.(js|jsx|mjs|css|json)$)': resolve(
'^.+\\.(gql|graphql)$': isEjecting
? '<rootDir>/node_modules/jest-transform-graphql'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this line just be 'jest-transform-graphql'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about that. This is just copying the pattern used above for babel-jest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Okay. Let's keep it for now.

@@ -687,6 +688,32 @@ Now running `npm start` and `npm run build` also builds Sass files.

`node-sass-chokidar` is used here as it addresses these issues.

## Adding GraphQL files
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section should be after "Adding Images, ..."

@petetnt
Copy link
Contributor Author

petetnt commented Feb 3, 2018

Thanks @iansu! 👌

"html-webpack-plugin": "2.30.1",
"identity-obj-proxy": "3.0.0",
"jest": "22.1.2",
"jest-transform-graphql": "2.1.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of frustrating that everybody will "inherit" whole three GQL dependencies after ejecting even if they don't use graphql.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree, but not sure what the other option would be.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's remove this dependency. All it does is literally one-line call to webpack loader, and we could do the same in our Jest transform file.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

@@ -729,6 +730,32 @@ Please be advised that this is also a custom feature of Webpack.
**It is not required for React** but many people enjoy it (and React Native uses a similar mechanism for images).<br>
An alternative way of handling static assets is described in the next section.

## Adding GraphQL files

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to have a note about it being supported only in 2.0.0+, like other similar features.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done ✅

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
@paulomcnally
Copy link

Is this available in the version 1.5.2?

@petetnt
Copy link
Contributor Author

petetnt commented Mar 6, 2018

@paulomcnally This feature will be available on the upcoming version 2 of CRA. If you want to load .graphql files in 1.5.2., you might want to consider https://github.com/timarney/react-app-rewired to modify the config of CRA to match this PR.

@paulomcnally
Copy link

Thanks for your answer @petetnt, I'll wait for version 2.x.

akstuhl pushed a commit to akstuhl/create-react-app that referenced this pull request Mar 15, 2018
* Add graphql loader to webpack config

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update README.md

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update react-scripts README.md

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add graphql jest transform

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add integration tests, pin versions in package.json

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Tests expect regexp matchers

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Use strict equal test instead

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Escaping is hard

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add comment for signifying a different file

* Update docs

* Fix jest config

* Remove node_modules exclusion

* Update README.md

* Inline graphql jest transform

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update copyright header

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Use .graphql extension only

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
yoiang pushed a commit to Adorkable-forkable/create-react-app-typescript that referenced this pull request May 2, 2018
patrick91 added a commit to patrick91/create-react-app-typescript that referenced this pull request May 14, 2018
Timer added a commit to Timer/create-react-app that referenced this pull request Sep 24, 2018
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
* Add graphql loader to webpack config

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update README.md

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update react-scripts README.md

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add graphql jest transform

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add integration tests, pin versions in package.json

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Tests expect regexp matchers

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Use strict equal test instead

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Escaping is hard

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Add comment for signifying a different file

* Update docs

* Fix jest config

* Remove node_modules exclusion

* Update README.md

* Inline graphql jest transform

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Update copyright header

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>

* Use .graphql extension only

Signed-off-by: petetnt <pete.a.nykanen@gmail.com>
zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
@silentlight
Copy link

silentlight commented Oct 10, 2018

I cannot seem to load .graphql files using CRA version 2.0.3.

Here are my packages in package.json

    "apollo-link": "^1.2.3",
    "apollo-link-error": "^1.1.1",
    "apollo-link-http": "^1.5.5",
    "axios": "^0.18.0",
    "babel-plugin-dynamic-import-node": "^2.0.0",
    "babel-plugin-syntax-dynamic-import": "^6.18.0",
    "babel-preset-es2015": "^6.24.1",
    "babel-preset-react-app": "^3.1.1",
    "compression": "^1.7.3",
    "cross-env": "^5.2.0",
    "env-cmd": "^8.0.2",
    "express": "^4.16.2",
    "file-loader": "^1.1.6",
    "graphql": "^14.0.2",
    "graphql-tag": "^2.10.0",
    "graphql.macro": "^1.0.2",
    "ignore-styles": "^5.0.1",
    "node-fetch": "^2.2.0",
    "pm2": "^3.2.2",
    "react": "^16.5.2",
    "react-dom": "^16.5.2",
    "react-loadable": "^5.3.1",
    "react-scripts": "^2.0.3",
    "styled-components": "^3.4.9",
    "url-loader": "^1.0.1"

Here is how I import it:

import pageDataQuery from '../../graphql/queries/pageData.graphql';

The error I am getting is Unexpected token inside of my .graphql file. Do I need to do anything else after installing graphql-tag? Any help would be greatly appreciated.

Thanks

@psamd
Copy link

psamd commented Oct 17, 2018

@silentlight the support for 'graphql-tag/loader' was removed #5076

Try using any one of these macros:

Or just eject your app and add the 'graphql-tag/loader' in your webpack config files (dev & prod)

// The GraphQL loader preprocesses GraphQL queries in .graphql files.
{
  test: /\.(graphql)$/,
  loader: 'graphql-tag/loader',
},

@chrisdostert
Copy link

why was this reverted? this functionality was awesome

@petetnt
Copy link
Contributor Author

petetnt commented Oct 18, 2018

Hi @chrisdostert!

The implicit loader was removed in favor of the macro approach, which gives much more flexibility (for example using different graphql-js version instead of the one shipped with CRA) with little to no code changes.

You can load .gql and .graphql files by installing the graphql-tag.macro from npm and using it like this:

import { loader } from 'graphql.macro';
const A = loader('./assets/graphql.graphql');

The loader statement gets replaced with the actual graphql-tag object on build time and the precomputed object also lowers performance overhead by bit!

@chrisdostert
Copy link

thx @petetnt for response.
This is opinionated, but personally the simplicity of single import & no additional direct dependency was quite nice & seemed to be more in line w/ the way CRA has approached features in general.
Again, opinionated.

@iansu
Copy link
Contributor

iansu commented Oct 18, 2018

As @petetnt mentioned, the main downside to this approach is that you're stuck with whatever version of graphql-js Create React App uses. And the only way to upgrade would be for CRA to update the version and then for you to upgrade to that new version of CRA in your project.

While using macros requires a bit of extra work we think the flexibility is worth the tradeoff. It also means we don't have to add loaders for every conceivable file type or for different tools (CSS-in-JS libraries for example).

@delmendo
Copy link

delmendo commented Nov 29, 2018

Feels like someone should put an alert here - the macro version hits a bug in babel so it never reloads your graphql files :(

#5580

@masull
Copy link

masull commented Dec 28, 2018

graphql.macro does not seem to work with a .graphql file that contains multiple operations.

./queries.graphql
query Query1 {
query1 {
...
query Query2 {
query2{

import {loader} from 'graphql.macro';
const {Query1, Query2} = loader('./queries.graphql');

Query1 and Query2 are undefined.

@lock lock bot locked and limited conversation to collaborators Jan 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.