From fe24d621f2a3e3ca6cec51ab549b30d5b259d661 Mon Sep 17 00:00:00 2001 From: Doug Parker Date: Wed, 8 Mar 2023 21:04:40 -0800 Subject: [PATCH] fix: always load JUnit reporter as ESM 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. --- .github/workflows/ci.yaml | 1 + e2e/jasmine_test/.bazelrc | 0 e2e/jasmine_test/BUILD.bazel | 1 + examples/commonjs/BUILD.bazel | 11 +++++++++++ examples/commonjs/lib.js | 3 +++ examples/commonjs/lib.spec.js | 7 +++++++ jasmine/defs.bzl | 1 + jasmine/private/BUILD.bazel | 3 ++- jasmine/private/jasmine_test.bzl | 12 ++++++++++++ jasmine/private/package.json | 1 + jasmine/repositories.bzl | 6 ++++++ 11 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 e2e/jasmine_test/.bazelrc create mode 100644 examples/commonjs/BUILD.bazel create mode 100644 examples/commonjs/lib.js create mode 100644 examples/commonjs/lib.spec.js create mode 100644 jasmine/private/package.json diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index facc12d..a8b9144 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -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) diff --git a/e2e/jasmine_test/.bazelrc b/e2e/jasmine_test/.bazelrc new file mode 100644 index 0000000..e69de29 diff --git a/e2e/jasmine_test/BUILD.bazel b/e2e/jasmine_test/BUILD.bazel index 8c6d8b8..e9a6382 100644 --- a/e2e/jasmine_test/BUILD.bazel +++ b/e2e/jasmine_test/BUILD.bazel @@ -8,4 +8,5 @@ jasmine_test( "lib.test.js", "package.json", ], + args = ["*.spec.js"], ) diff --git a/examples/commonjs/BUILD.bazel b/examples/commonjs/BUILD.bazel new file mode 100644 index 0000000..9c288b0 --- /dev/null +++ b/examples/commonjs/BUILD.bazel @@ -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(), +) diff --git a/examples/commonjs/lib.js b/examples/commonjs/lib.js new file mode 100644 index 0000000..801a115 --- /dev/null +++ b/examples/commonjs/lib.js @@ -0,0 +1,3 @@ +module.exports = { + say_hi: () => "hi", +}; diff --git a/examples/commonjs/lib.spec.js b/examples/commonjs/lib.spec.js new file mode 100644 index 0000000..6debdf7 --- /dev/null +++ b/examples/commonjs/lib.spec.js @@ -0,0 +1,7 @@ +const { say_hi } = require("./lib.js"); + +describe("lib", () => { + it("should say hi", () => { + expect(say_hi()).toBe("hi"); + }); +}); diff --git a/jasmine/defs.bzl b/jasmine/defs.bzl index c67bfde..3b5fd65 100644 --- a/jasmine/defs.bzl +++ b/jasmine/defs.bzl @@ -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), diff --git a/jasmine/private/BUILD.bazel b/jasmine/private/BUILD.bazel index 55d7a1c..2a2a1b2 100644 --- a/jasmine/private/BUILD.bazel +++ b/jasmine/private/BUILD.bazel @@ -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*.*.*/* diff --git a/jasmine/private/jasmine_test.bzl b/jasmine/private/jasmine_test.bzl index 751a619..9edf34e 100644 --- a/jasmine/private/jasmine_test.bzl +++ b/jasmine/private/jasmine_test.bzl @@ -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"], @@ -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 diff --git a/jasmine/private/package.json b/jasmine/private/package.json new file mode 100644 index 0000000..0967ef4 --- /dev/null +++ b/jasmine/private/package.json @@ -0,0 +1 @@ +{} diff --git a/jasmine/repositories.bzl b/jasmine/repositories.bzl index eafc981..d360933 100644 --- a/jasmine/repositories.bzl +++ b/jasmine/repositories.bzl @@ -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": [