Skip to content

Commit

Permalink
fix: always load JUnit reporter as ESM
Browse files Browse the repository at this point in the history
At runtime, the JUnit reporter exists at `.../execroot/${wksp}/bazel-out/${cfg}/bin/external/jasmine/junit_reporter.js` with no included `package.json`. This means Node walks up that tree looking for a `package.json` file and typically won't find one. However, if users include a `//:package.json` file in their own workspace, it will be visible at `.../execroot/${wksp}/bazel-out/${cfg}/bin/package.json`, meaning it will be picked up as owning `junit_reporter.js`. This means that any user-specified `type: "module"` applies to `junit_reporter.js`, and caused it to be loaded as ESM, even when it was authored in CommonJS.

The solution here is to copy over a `package.json` we own to `external/jasmine/package.json`. This way any JavaScript loaded will have a stable `package.json` which uses CommonJS.

Ironically, `//e2e/jasmine_test/` actually already exercised this exact test case, but wasn't running on CI. So this commit also re-enables that test and fixes it. Actually all the test cases use ESM, so I also added `//examples/commonjs/...` and left the "default" behavior there to ensure that doesn't break in the future.
  • Loading branch information
dgp1130 committed Mar 9, 2023
1 parent 1a5d126 commit fe24d62
Show file tree
Hide file tree
Showing 11 changed files with 45 additions and 1 deletion.
1 change: 1 addition & 0 deletions .github/workflows/ci.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ jobs:
bazelversion: ${{ fromJSON(needs.matrix-prep-bazelversion.outputs.bazelversions) }}
folder:
- "."
- "e2e/jasmine_test"
- "e2e/workspace"
exclude:
# Don't test macos with Bazel 5 to minimize macOS minutes (billed at 10X)
Expand Down
Empty file added e2e/jasmine_test/.bazelrc
Empty file.
1 change: 1 addition & 0 deletions e2e/jasmine_test/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ jasmine_test(
"lib.test.js",
"package.json",
],
args = ["*.spec.js"],
)
11 changes: 11 additions & 0 deletions examples/commonjs/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
load("@aspect_rules_jasmine//jasmine:defs.bzl", "jasmine_test")

jasmine_test(
name = "test",
data = [
"lib.js",
"lib.spec.js",
],
args = ["*.spec.js"],
chdir = package_name(),
)
3 changes: 3 additions & 0 deletions examples/commonjs/lib.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
module.exports = {
say_hi: () => "hi",
};
7 changes: 7 additions & 0 deletions examples/commonjs/lib.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const { say_hi } = require("./lib.js");

describe("lib", () => {
it("should say hi", () => {
expect(say_hi()).toBe("hi");
});
});
1 change: 1 addition & 0 deletions jasmine/defs.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ def jasmine_test(jasmine_repository = "jasmine", **kwargs):
}),
entry_point = "@{}//:jasmine_entrypoint".format(jasmine_repository),
junit_reporter = "@{}//:junit_reporter".format(jasmine_repository),
package_json = "@{}//:package_json".format(jasmine_repository),
data = kwargs.pop("data", []) + [
"@{}//:node_modules/jasmine".format(jasmine_repository),
"@{}//:node_modules/jasmine-core".format(jasmine_repository),
Expand Down
3 changes: 2 additions & 1 deletion jasmine/private/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
load("@bazel_skylib//:bzl_library.bzl", "bzl_library")

exports_files([
"junit_reporter.js",
"package.json",
"runner.js",
"junit_reporter.js"
])

# gazelle:exclude v*.*.*/*
Expand Down
12 changes: 12 additions & 0 deletions jasmine/private/jasmine_test.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,17 @@ _attrs = dicts.add(js_binary_lib.attrs, {
mandatory = True,
allow_single_file = True,
),
# We include a `package.json` not because it does anything particularly
# useful (it doesn't), but because without it, a `package.json` file in the
# root of the user's workspace (`//:package.json`) would be used for any
# JavaScript invoked in the `@jasmine` workspace. This could break things by
# changing the default type to `commonjs` or adding `imports` / `exports`.
# Including a `package.json` we own in the test execution, even if empty,
# prevents users from accidentally breaking the tests.
"package_json": attr.label(
mandatory = True,
allow_single_file = [".json"],
),
"config": attr.label(
doc = "jasmine config file. See: https://jasmine.github.io/setup/nodejs.html#configuration",
allow_single_file = [".json", ".js"],
Expand All @@ -16,6 +27,7 @@ _attrs = dicts.add(js_binary_lib.attrs, {
def _impl(ctx):
files = ctx.files.data[:]
files.append(ctx.file.junit_reporter)
files.append(ctx.file.package_json)

junit_reporter = ctx.file.junit_reporter.short_path

Expand Down
1 change: 1 addition & 0 deletions jasmine/private/package.json
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
{}
6 changes: 6 additions & 0 deletions jasmine/repositories.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ If you need custom versions, please file an issue.""".format(jasmine_version, TO
src = "@aspect_rules_jasmine//jasmine/private:junit_reporter.js",
out = "junit_reporter.js",
visibility = ["//visibility:public"],
)""",
"""copy_file(
name = "package_json",
src = "@aspect_rules_jasmine//jasmine/private:package.json",
out = "package.json",
visibility = ["//visibility:public"],
)""",
],
"defs.bzl": [
Expand Down

0 comments on commit fe24d62

Please sign in to comment.