-
Notifications
You must be signed in to change notification settings - Fork 522
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
feat: support multiple node_modules roots in require_patch #3051
feat: support multiple node_modules roots in require_patch #3051
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
e1f24aa
to
75e8c8a
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
75e8c8a
to
ae1aa6a
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I will need to add some documentation around this feature. The limitation currently is that you must define the two node modules roots under different paths with |
ae1aa6a
to
1dd9de2
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
1dd9de2
to
9764cbf
Compare
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Refs: #266
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
What is the current behavior?
When referencing multiple
node_modules
from(npm|yarn)_install
targets in anodejs_(binary|target)
, you won't be able to resolve modules from allnode_modules
roots.For example, having a separate
yarn_install
:alongside the root:
and referring to both of them in a test rule:
would result in the test run not being able to access the
jest
module.Issue Number: #266
What is the new behavior?
Resolution of a
require
call will be done against allnode_modules
roots. The behavior is the same if it is not found.Does this PR introduce a breaking change?
The prior behavior would have allowed for node_modules to be included without being able to be imported. Due to this, if anyone were relying on this undefined behavior to include
node_modules
data where importing that would have thrown an error, but I can't imagine a use case where this would be useful behavior to rely on.Other information
Some minor adjustments to test case names such as "mode" updated to "module".