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

Node loader resolve improvements #253

Merged
merged 2 commits into from
Jul 26, 2018

Conversation

gregmagolan
Copy link
Collaborator

Hermetic resolve in node_loader.js for nodejs_binary & nodejs_test.

Resolves now in the following order:

  1. build-in modules (such as fs, path, etc...)

  2. relative and absolute imports (node can resolve these with request directly from the parent.filename for relative imports without needing to use the runfiles path)

  3. npm dependencies within the parent.filename node_modules (this is the case when an npm package N1/node_modules/A imports another dependent npm package within the same node_modules such as N1/node_modules/B or N1/node_modules/A/node_modules/B). Protection against unintentionally importing from another node_modules further down the path tree such as N2/node_modules/B is in place. This can happen if the workspace has a node_modules folder in it but the workspace node_modules is not intended to be in the runfiles (it is not the node_modules filegroup specified for the operation).

  4. runfiles import

  5. external repository node_modules import (if the parent file is from an external repository)

  6. node_modules filegroup import

  7. module root (unchanged)

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix #251 after this is in (so we don't have merge conflicts)

@@ -0,0 +1,31 @@
# These options are enabled when running on CI
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split out unrelated changes from the PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 #255

var_3: &setup-bazel-remote-cache
run:
name: Start up bazel remote cache proxy
command: ~/bazel-remote-proxy -backend circleci://
background: true
# Move node binaries out of the way to enforce that Bazel uses
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make separate cleanup PRs that can land first

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 #255

# node_loader_preserve_symlinks tests. We need to run bazel from within
# the WORKSPACE to ensure that the preserve_symlinks setting for both
# is honored
- run: 'cd internal/e2e/node_loader_no_preserve_symlinks &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's make package.json "test" script which does the steps

Copy link
Collaborator Author

@gregmagolan gregmagolan Jul 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@gregmagolan gregmagolan force-pushed the node-loader-resolve branch 6 times, most recently from e9b6ff3 to 62dd07e Compare July 25, 2018 21:46
@gregmagolan
Copy link
Collaborator Author

Usage for external repository nested node_modules resolves looks like this for the main @//:node_modules filegroup:

filegroup(
    name = "node_modules",
    srcs = glob(["node_modules/**/*"]) + ["@node_resolve_dep//:node_modules"],
)

The external repository defines its own node_modules filegroup @node_resolve_dep//:node_modules which is added to the user's node_modules filegroup as shown above.

@gregmagolan gregmagolan force-pushed the node-loader-resolve branch from 62dd07e to d8b7406 Compare July 26, 2018 16:12
@alexeagle alexeagle force-pushed the node-loader-resolve branch from d8b7406 to 02443b4 Compare July 26, 2018 20:13
@alexeagle alexeagle force-pushed the node-loader-resolve branch from 02443b4 to fd8d47d Compare July 26, 2018 20:17
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed in person, this change introduces a need for users to stitch together node modules from nested workspaces so they're not excluded from the glob. At present, rules_typescript is the only one of these.

We'll add a breaking change PR to rules_typescript that also asserts on the latest version of rules_nodejs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants