-
Notifications
You must be signed in to change notification settings - Fork 56
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
Support for node_modules in graphql imports #136
Conversation
Can you please rebase so I can merge? Also, can you please add a test case for this? You might need to add another |
@schickling Sure, I'm working on it |
@schickling How should be done the test case ? I can probably install a package as a dev dependency that has graphql files and import one of those, or can you see a better way ? |
You can also "manually create" a |
Tests added, please keep in mind that the following code will still not work: importSchema('module-name/a.graphql') It would introduce a breaking change |
The tests still seem to fail. Can you look into it and fix it? |
yep, checks for node are failing here I already pushed that folder and tests are passing, do you have any idea of how can I replicate that behaviour ? |
@lfades It might be possible the CI does something weird with caching node_modules. I suggest creating the |
@SpaceK33z that makes a lot of sense, I'll add some |
@schickling I found some bad news about this PR, it only works in Node > 8, I already know the solution but I don't like it, so I want to share my thoughts with you first: require.resolve is the main change done by this PR, the second param that it receives it's an object that has a Possible solutions: |
The issue that lead to that feature being added to Node 8 lists a bunch of packages in the Prior Art section. |
@tamlyn This seems to be the way to go. @lfades It'd be awesome to see this in graphql-import and if you'd need help to bring this PR up to speed, I'm offering myself as a tribute cc @schickling any objections to using |
If |
Still very much looking forward to this feature. Perhaps just finish the PR with the |
Closed in favor of #216 |
This is a continuation of PR #136 from @lfades (spoke to @lfades about this and he was okay with it). It uses `resolve-from` instead of `require.resolve` with the `paths` option because it is not compatible with Node < 8. Also I think this one works a bit differently; it doesn't introduce a breaking change since it will first try to look up the path relative to the file the import is in, and only if that fails it will use `resolve-from`. Fixes #57
This is a continuation of PR #136 from @lfades (spoke to @lfades about this and he was okay with it). It uses `resolve-from` instead of `require.resolve` with the `paths` option because it is not compatible with Node < 8. Also I think this one works a bit differently; it doesn't introduce a breaking change since it will first try to look up the path relative to the file the import is in, and only if that fails it will use `resolve-from`. Fixes #57
Before (manual search for
node_modules
folder):# import Auth from '../../node_modules/module-name/auth.graphql'
Now:
# import Auth from 'module-name/auth.graphql'