Skip to content
This repository has been archived by the owner on Jan 26, 2019. It is now read-only.

Add loader for .graqhql files #270

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

patrick91
Copy link
Contributor

@patrick91 patrick91 force-pushed the feature/loader-graphql branch from 33f23ff to 6bd2327 Compare March 3, 2018 22:22
@bahkostya
Copy link

Can somebody from maintainers look at this pull request Please? @DorianGrey, @wmonk
Thanks in advance

@harrisrobin
Copy link

Yes, need this please.

Thank you!

Copy link
Collaborator

@DorianGrey DorianGrey left a comment

Choose a reason for hiding this comment

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

I'm very sorry for this delayed reponse - I've been quite busy the last couple of weeks and didn't have the time to look into this as precisely as it deserves.

Most things are looking fine, apart from the two issues mentioned (where only one is critical).

Side note: @wmonk , even though this would be "pre-picking" of a CRA 2.x feature, I suppose it's not that unlikely, esp. since it cuts down a bit of merging work for that version ... ;)

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

Choose a reason for hiding this comment

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

Is there any particular reason that this loader is only registered in dev, but not in prod mode? (See the webpack.config.prod.js)

@@ -677,6 +678,34 @@ 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

> Note: this feature is available with [email protected] and higher.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suppose this note is not required here, since this PR results in some kind of "pre-picking".

@simsim0709
Copy link

Hi, @patrick91

Are you still working on this pr? If not, I would like to make a pr again, cuz I need it for my project.

Thank you

@patrick91
Copy link
Contributor Author

I'll finish this in the weekend if that's ok :)

@simsim0709
Copy link

Yop. No problem. Thank you for this pr!! 😀

@eddiemoore
Copy link

Any progress on this?

@patrick91 patrick91 force-pushed the feature/loader-graphql branch from 4f61b6b to 0914932 Compare May 14, 2018 17:46
@patrick91
Copy link
Contributor Author

Sorry folks! I was travelling, I just pushed the changes, thanks for the nudges @eddiemoore @simsim0709 :)

@DorianGrey this should be fine to merge now, as soon as I fix the failure on travis ci :)

@patrick91 patrick91 force-pushed the feature/loader-graphql branch from 0914932 to d3716d5 Compare May 14, 2018 20:29
@patrick91
Copy link
Contributor Author

Ok, I'm not sure why it fails, @DorianGrey can you tell me how to run the single failing test locally? I had troubles with docker unfortunately

@DorianGrey
Copy link
Collaborator

You may just run tasks/e2e-kitchensink.sh in the project root. Beware that this is a bit unstable in case it is executed multiple times in a non-isolated environment, since the scripts are using a local yarn repository to publish the packages, and that may cause version collisions. However, due to the same issue, its a bit complicated to execute one of the test cases of that suite in isolation.

The particular test that seems to fail in your case is this one: https://github.com/wmonk/create-react-app-typescript/blob/master/tasks/e2e-kitchensink.sh#L144
Yet I'm a bit surprised of why - the result indicates that the graphql file was picked up via file-loader, which should not be responsible regarding the webpack configuration.

@patrick91
Copy link
Contributor Author

@DorianGrey yeah, I saw that, I was able to run the kitchensink locally but with no luck, it seems like it using an old version of the scripts, I even tried to delete the config and it didn't break, mmh, let me try again

@patrick91 patrick91 force-pushed the feature/loader-graphql branch from ad4613d to d3716d5 Compare May 16, 2018 21:54
@patrick91
Copy link
Contributor Author

@DorianGrey it seems that we need to pass --scripts-version=react-scripts-ts to

npx create-react-app --internal-testing-template="$root_path"/packages/react-scripts/fixtures/kitchensink test-kitchensink

so it will load the correct scripts, but the test fails still, with a different failure

/v/f/g/_/T/t/test-kitchensink (py3.6) REACT_APP_SHELL_ENV_MESSAGE=fromtheshell \
  NODE_PATH=src \
  PUBLIC_URL=http://www.example.org/spa/ \
  yarn build


yarn run v1.6.0
$ react-scripts-ts build
fs.js:987
  return binding.stat(pathModule._makeLong(path));
                 ^

Error: ENOENT: no such file or directory, stat '/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/tsconfig.json'
    at Error (native)
    at Object.fs.statSync (fs.js:987:18)
    at resolveConfigPath (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths/lib/tsconfig-loader.js:41:12)
    at loadSyncDefault (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths/lib/tsconfig-loader.js:19:22)
    at tsConfigLoader (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths/lib/tsconfig-loader.js:13:22)
    at configLoader (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths/lib/config-loader.js:27:22)
    at Object.loadConfig (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths/lib/config-loader.js:8:12)
    at new TsconfigPathsPlugin (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/tsconfig-paths-webpack-plugin/lib/plugin.js:20:42)
    at Object.<anonymous> (/private/var/folders/gy/_nycm7nx4zd45ldqdz5lzhk40000gn/T/tmp.hTn8c68y/test-kitchensink/node_modules/react-scripts-ts/config/webpack.config.prod.js:134:7)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.require (module.js:497:17)
    at require (internal/module.js:20:19)
error Command failed with exit code 1.

@DorianGrey
Copy link
Collaborator

Hm... I'm not sure about adding this, since --internal-testing-template refers to a particular template that is used for the tests (in packages/react-scripts/fixtures/kitchensink). That particular template does not include any typescript related stuff.
That's why adding --scripts-version=react-scripts-ts does not work for that test: this parameter overwrites the template used for the test, thus there is no tsconfig.json as the build script expects.

@patrick91
Copy link
Contributor Author

Yup, but without it we are testing the wrong scripts right? I'm happy to do another PR with the updated tests if that's ok :)

@DorianGrey
Copy link
Collaborator

DorianGrey commented May 17, 2018

The --internal-testing-template overwrites the template used during the init process on project generation, ignoring the one caused by the --scripts-version option. The resulting project contains this enforced template, but still utilizes the build configuration provided by the scripts package.

It might take a little more to set this up properly, i.e. not only modifying the generation command, but the whole particular template. PR is welcome, though 👍

@heresandyboy
Copy link

Hi, I have been trying to figure out how to use .graphql files in create-react-app-typescript. My investigations have led me to this PR - was support for loading .graphql ever implemented? Are there any workarounds available? My main reason for switching from .ts files using gql`` is so that I can easily use fragments.

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

Successfully merging this pull request may close these issues.

7 participants