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

Support for node_modules in graphql imports #136

Closed
wants to merge 6 commits into from

Conversation

lfades
Copy link

@lfades lfades commented Apr 13, 2018

Before (manual search for node_modules folder):

# import Auth from '../../node_modules/module-name/auth.graphql'

Now:

# import Auth from 'module-name/auth.graphql'

@schickling
Copy link
Contributor

Can you please rebase so I can merge?

Also, can you please add a test case for this? You might need to add another .gitignore file.

@lfades
Copy link
Author

lfades commented Apr 26, 2018

@schickling Sure, I'm working on it

@lfades
Copy link
Author

lfades commented Apr 26, 2018

@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 ?

@schickling
Copy link
Contributor

You can also "manually create" a node_modules folder + an example package in the corresponding test folder and explicitly allow it in the .gitignore. This way you don't need to install it as a dependency.

@lfades
Copy link
Author

lfades commented Apr 30, 2018

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

@lfades lfades mentioned this pull request May 4, 2018
@schickling
Copy link
Contributor

The tests still seem to fail. Can you look into it and fix it?

@lfades
Copy link
Author

lfades commented May 10, 2018

yep, checks for node are failing here

image

I already pushed that folder and tests are passing, do you have any idea of how can I replicate that behaviour ?

@SpaceK33z
Copy link
Collaborator

SpaceK33z commented May 10, 2018

@lfades It might be possible the CI does something weird with caching node_modules. I suggest creating the graphql-import-test/b.graphql file before starting the relevant test, that way you are sure it is added. Also, this way the b.graphql file is not gone after you do rm -rf node_modules.

@lfades
Copy link
Author

lfades commented May 10, 2018

@SpaceK33z that makes a lot of sense, I'll add some fs code to the test 💃

@lfades
Copy link
Author

lfades commented May 10, 2018

@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 paths option, which I use to make the import relative to the graphql file. The reason why it doesn't works in previous versions of node is because such option doesn't exists.

Possible solutions:

  • By using internal code like the way eslint is doing it here, related to this issue. I don't personally like this solution because we'll lose types and we'll need to use internal methods e.g _.something, but is an option
  • Only allow module imports in newest versions of node

@tamlyn
Copy link

tamlyn commented May 24, 2018

The issue that lead to that feature being added to Node 8 lists a bunch of packages in the Prior Art section. resolve-from gets >5m downloads a week and would probably be a good choice.

@kitten
Copy link

kitten commented Jun 18, 2018

@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 resolve-from or this PR being picked up again? Btw #4 should probably reopened as this PR addresses it

@lfades
Copy link
Author

lfades commented Jun 18, 2018

If resolve-from is okay I can finish this PR with it

@SpaceK33z
Copy link
Collaborator

Still very much looking forward to this feature. Perhaps just finish the PR with the resolve-from as it seems to be the only way? I can help if you don't have the time.

@lfades
Copy link
Author

lfades commented Aug 11, 2018

Closed in favor of #216

@lfades lfades closed this Aug 11, 2018
SpaceK33z added a commit that referenced this pull request Sep 7, 2018
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
SpaceK33z added a commit that referenced this pull request Sep 7, 2018
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants