-
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
fix(esbuild): add link_workspace_root for workspace absolute imports #2476
Conversation
packages/esbuild/esbuild.bzl
Outdated
@@ -15,6 +15,9 @@ def _esbuild_impl(ctx): | |||
# how to resolve custom package or module names | |||
path_alias_mappings = dict() | |||
|
|||
if (ctx.attr.link_workspace_root): | |||
path_alias_mappings.update(generate_path_mapping(ctx.workspace_name, "/".join(ctx.build_file_path.split("/")[:-1]))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would add:
"build_bazel_rules_nodejs": [
"packages/esbuild/test/typescript/index.mjs",
"packages/esbuild/test/typescript"
],
"build_bazel_rules_nodejs/*": [
"packages/esbuild/test/typescript/*"
]
to the path mappings. Wouldn't these need to point to the root of the workspace, which would be './*'
rather than the root of where the esbuild
rule is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right... the import examples in the test should be build_bazel_rules_nodejs/packages/esbuild/test/typescript/...subfolder...
. I think that's actually what I originally had but was getting errors and confused (all because of a silly typo), and this is what came out of the confusion in the end...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated, I think that's right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test this now creates:
"paths": {
"build_bazel_rules_nodejs": [
"./index.mjs",
"."
],
"build_bazel_rules_nodejs/*": [
"./*"
]
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that looks correct, nice one!
72bcfde
to
75e47b5
Compare
Fixes #2474