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): set correct base url when rule is at root #2506

Merged
merged 1 commit into from
Mar 6, 2021

Conversation

mattem
Copy link
Collaborator

@mattem mattem commented Mar 5, 2021

When the esbuild rule is in the root BUILD.bazel file, rule_path is empty, splitting on this results in a list with one empty string, which is then translated into one path segment. When rule_path is empty, the rule is already in the correct base path.

This also adds an npm dependency to the example to exercise node_module resolution when bundling.

closes #2503

@@ -7,6 +7,9 @@
"tslib": "1.9.0",
"typescript": "3.5.3"
},
"dependencies": {
"chalk": "4.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should a package-lock.json also be changing...?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah looks like we should be using npm ci when we install deps for examples

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nice catch @jbedard, I should have regenerated the lock file, will update.
Currently we can't use npm ci during install of the deps in integration tests as the package replacement gets in the way, or is that not what you meant?

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.

LGTM other than the problem @jbedard points out

@@ -7,6 +7,9 @@
"tslib": "1.9.0",
"typescript": "3.5.3"
},
"dependencies": {
"chalk": "4.1.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

yeah looks like we should be using npm ci when we install deps for examples

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: error: Could not resolve "react" (mark it as external to exclude it from the bundle)
3 participants