Skip to content

fix(esbuild): set correct base url when rule is at root#2506

Merged
mattem merged 1 commit into
bazel-contrib:stablefrom
mattem:fix/esbuild_base_url
Mar 6, 2021
Merged

fix(esbuild): set correct base url when rule is at root#2506
mattem merged 1 commit into
bazel-contrib:stablefrom
mattem:fix/esbuild_base_url

Conversation

@mattem

@mattem mattem commented Mar 5, 2021

Copy link
Copy Markdown
Collaborator

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

"typescript": "3.5.3"
},
"dependencies": {
"chalk": "4.1.0"

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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?

@alexeagle alexeagle left a comment

Copy link
Copy Markdown
Collaborator

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

"typescript": "3.5.3"
},
"dependencies": {
"chalk": "4.1.0"

Copy link
Copy Markdown
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