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

fix(esbuild): add link_workspace_root for workspace absolute imports #2476

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

jbedard
Copy link
Collaborator

@jbedard jbedard commented Feb 18, 2021

Fixes #2474

@@ -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])))
Copy link
Collaborator

@mattem mattem Feb 18, 2021

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?

Copy link
Collaborator Author

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...

Copy link
Collaborator Author

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?

Copy link
Collaborator Author

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/*": [
                "./*"
            ]
        }

Copy link
Collaborator

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!

@mattem mattem merged commit ba7e48e into bazel-contrib:stable Feb 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

esbuild can't resolve workspace absolute imports
2 participants