feat: allow running NPM tools from execroot #2297
Conversation
NPM tools are currently run out of external/{md}/node_modules. If the
tool loads user-provided files in the execroot, which is the common
case, then this can lead to bad interactions.
Two examples of bad interactions are:
1. A tool that loads React and the user code too,
hooks becoming unusable.
2. A tool that calls into a bundler like Webpack,
potentially bundling duplicate packages.
Signed-off-by: Duarte Nunes <duarte@hey.com>
duarten
requested review from
alexeagle,
gregmagolan,
mattem and
soldair
as code owners
| # TODO: after we link-all-bins we should not need this extra lookup | ||
| if [[ ! -f "$MAIN" ]]; then | ||
| if [ "$FROM_EXECROOT" = true ]; then | ||
| MAIN="$EXECROOT/$MAIN" |
There was a problem hiding this comment.
does something break if we always use FROM_EXECROOT? do you understand what if anything still relies on the old path?
There was a problem hiding this comment.
Tests seem to pass except for //internal/node/test:main_test when executed with bazel test; it works fine with bazel run. Maybe this is because paths change when running in the sandbox? The code could probably be adapted to also deal with that case
There was a problem hiding this comment.
okay I think we should immediately attempt to get the default flipped in a PR following this one.
reasoning:
- will have to wait 6mo otherwise
- long flag flip rollouts are a bummer
- in the meantime if other users hit the same bug you did it will be really hard for them to discover this opt-in flag that fixes it
| if [ "$FROM_EXECROOT" = true ]; then | ||
| MAIN="$EXECROOT/$MAIN" | ||
| else | ||
| MAIN=TEMPLATED_entry_point_manifest_path |
There was a problem hiding this comment.
is it possible to make the same logical fix in the .bzl file that calls the launcher, rather than change the shell script? If it is, then that's the better layer to make the change
There was a problem hiding this comment.
That would be _nodejs_binary_impl. I wanted to reuse the logic to detect the execroot path, which is in the launcher. I'm not sure if there's another way to calculate it, or if it's even possible to move that logic to the rule implementation, since it seems to be based around detecting whether some paths exist.
There was a problem hiding this comment.
@alexeagle any further comment here?
There was a problem hiding this comment.
yeah I think that makes sense.
|
ping :) |
NPM tools are currently run out of external/{md}/node_modules. If the
tool loads user-provided files in the execroot, which is the common
case, then this can lead to bad interactions.
Two examples of bad interactions are:
1. A tool that loads React and the user code too,
hooks becoming unusable.
2. A tool that calls into a bundler like Webpack,
potentially bundling duplicate packages.
Signed-off-by: Duarte Nunes <duarte@hey.com>
NPM tools are currently run out of external/{md}/node_modules. If the
tool loads user-provided files in the execroot, which is the common
case, then this can lead to bad interactions.
Two examples of bad interactions are:
hooks becoming unusable.
potentially bundling duplicate packages.
Signed-off-by: Duarte Nunes duarte@hey.com
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?
Issue Number: #2286
What is the new behavior?
A user-provided flag controls where the NPM tool is loaded from.
Does this PR introduce a breaking change?
Other information
Tests and docs are missing, looking for early feedback.