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

Forward all args to originalResolveFilename #778

Merged
merged 6 commits into from
May 22, 2019

Conversation

Globegitter
Copy link
Contributor

@Globegitter Globegitter commented May 22, 2019

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

What kind of change does this PR introduce?

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

If I call require.resolve("../test.js", { paths: ["/some/path/in/sandbox"] }) the options are not forwarded correctly because the _resolveFilename method takes 4 parameters.

What is the new behavior?

All expected parameters are forwarded, so are the options now and paths will be respected.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I ran into this as the latest version of one of our npm dependencies is making use of this case and that fix fixes it.

@Globegitter Globegitter changed the title Fforward all args to originalResolveFilename Forward all args to originalResolveFilename May 22, 2019
@alexeagle alexeagle merged commit c77f5f1 into bazel-contrib:master May 22, 2019
@Globegitter Globegitter deleted the patch-3 branch May 22, 2019 20:18
@Globegitter Globegitter mentioned this pull request May 30, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants