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

Further node resolve filename fix #784

Closed

Conversation

Globegitter
Copy link
Collaborator

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

Another fix of very corner case module importing of a usage that I found in one of our dependencies: https://github.com/nuxt/nuxt.js/blob/91c3642e64166a939683b856e5b34ef42d12bb0e/packages/core/src/resolver.js#L26

I did also submit a possible fix to the project itself: nuxt/nuxt#5796 but not sure if it will get accepted at this stage.

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

     Module._resolveFilename(npm_module, {
        paths: ["..],
      })

does not work because of the checks done in the custom _resolveFilename method

What is the new behavior?

Node.js seems to allow a call of:

     Module._resolveFilename(npm_module, {
        paths: ["..],
      })

which seems to behave very similar to require.resolve(npm_module, {paths: [".."]}) but it does work different internally as the first invocation sets the paths of the parent rather than setting options. So now we check if parent.paths has been set and special case that import so it can work.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Not a 100% sure if we should really support this but I guess it does work with vanilla nodejs, so it may be worth it.

@Globegitter
Copy link
Collaborator Author

@alexeagle any idea why these changes (as well as the ones from #778) would fail on windows only? As far as I can see the code is platform independent and I also do not have a windows laptop to debug.

@github-actions
Copy link

This Pull Request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs in two weeks. Collaborators can add a "cleanup" or "need: discussion" label to keep it open indefinitely. Thanks for your contributions to rules_nodejs!

@github-actions github-actions bot added the Can Close? We will close this in 30 days if there is no further activity label Oct 15, 2020
@github-actions
Copy link

This PR was automatically closed because it went two weeks without a reply since it was labeled "Can Close?"

@github-actions github-actions bot closed this Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Can Close? We will close this in 30 days if there is no further activity cla: yes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants