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(builtin): fix a bug where mjs entry points were not added to runfiles #3406

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

kormide
Copy link
Collaborator

@kormide kormide commented Apr 13, 2022

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: Generally related to #3277

Calling bazel run on a nodejs_binary target that doesn't add its entry_point to data will cause node to run with no script.

What is the new behavior?

The script is run properly.

Does this PR introduce a breaking change?

  • Yes
  • No

# entry point is only needed in runfiles if it is a .js file
if len(ctx.files.entry_point) == 1 and ctx.files.entry_point[0].extension == "js":
# entry point is only needed in runfiles if it is a javascript file
if len(ctx.files.entry_point) == 1 and ctx.files.entry_point[0].extension in ["js", "mjs"]:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe we could share a constant list with some other location like is_js_file helper or something? it feels always hard to know all the spots that need updates

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

@@ -3,14 +3,12 @@ load("@build_bazel_rules_nodejs//:index.bzl", "nodejs_binary")
nodejs_binary(
name = "has_deps",
data = [
Copy link
Collaborator

Choose a reason for hiding this comment

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

ah, this should have been our clue

@alexeagle alexeagle merged commit e24473c into stable Apr 14, 2022
@alexeagle alexeagle deleted the run-esm-bug branch April 14, 2022 00:17
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.

2 participants