-
Notifications
You must be signed in to change notification settings - Fork 518
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Loading from multiple node_modules with version > 4.4.1 #3067
Comments
This is still happening with version 4.6.0. @alexeagle, @gregmagolan any tips on how to fix this would be greatly appreciated :) |
hi sorry, we've been 100% heads-down on the 5.x breaking changes and getting that release out. I doubt this is fixed there but we should at least verify it's still reproducible on 5.0.0-rc.0 which I'm going to cut todayish. |
It still reproduces with 5.0.0-rc.1. I've updated the reproducer here. |
Could this possibly be related to my issue here? 馃 #3340 |
It could be! If you are on 5.x.x it may also be related to #3264, so you should try to set |
@duarten Thanks for the clue! 馃檹 Indeed solved the problem. Spend most of Friday trying to hunt this one out. If you understand the mechanics behind the problem: will it be solved when #3264 is taken care of? I have a hard time figuring out if this is a bug or a side-effect of an optimisation. Anyway if it's the latter, it shouldn't be the default behaviour at least... :) |
I think it's a bug and will be fixed in #3264, but I don't understand what exactly is causing the issue, I just had recently spent some time trying to figure a problem that was caused by it :) |
Fixed by #3380 |
Ah sorry... |
I checked the repo rebased to stable HEAD. This looks like a side-effect of In the call stack you can see it resolving into a deeply nested node_modules via the
If I set that to false then the test passes,
Is the |
It was intentional at some point, but it is indeed no longer necessary. Happy to close the issue unless there's something to attach to it like docs update or something. |
No longer in scope for rules_nodejs which only supplies the Node.js toolchain as of v6.0.0. Downstream canonical JavaScript + Node.js ruleset is now https://github.com/aspect-build/rules_js. |
馃悶 bug report
Affected Rule
nodejs_binary
/nodejs_test
Is this a regression?
Yes, 4.4.1 works fine, but 4.4.2 and 4.4.3 are broken. This is likely related to the linker refactor.
Description
When a package is symlinked under node_modules due to, for example, the
packge_name
attribute ofjs_library
, then node modules will be loaded from a different directory.I noticed this with react, which complains when multiple instances are loaded.
We can see the problem from the following (edited) stack trace:
Notice how we first load react from
node_modules/react-dom
, and later fromnode_modules/repro/node_modules
.The component package contains the following rule:
Omitting the workspace name (repro) from the package_name seems to solve the problem (i.e., changing it to "component" or "blah/component").
馃敩 Minimal Reproduction
I committed a reproduction here.
馃敟 Exception or Error
In this case, react complains and the test fails.
馃實 Your Environment
Operating System:
Output of
bazel version
:Rules_nodejs version:
The text was updated successfully, but these errors were encountered: