refactor: remove vendored jasmine versions, user must provider their own jasmine from their node_modules

[gregmagolan]

In the same spirit as aspect-build/rules_webpack#110

[gregmagolan]

@jbedard the behind the scenes package_json attribute doesn't seem possible with this pattern but maybe the problem goes away entirely when switching to use the user's jasmine?

[jbedard]

This is only for tests now? WDYT about putting it alongside tests so it's clear it's only for tests, and also to prepare for testing with multiple versions...

[jbedard]

What if we don't want any reporting?

[gregmagolan]

Its on by default and can't be disabled. It's required to get junit xml out that Bazel consumes.

[jbedard]

Previously the user didn't need to know that though :(

[gregmagolan]

That is not a change from the current behaviour where the attribute is mandatory,

    "junit_reporter": attr.label(
        mandatory = True,
        allow_single_file = True,
    ),

[gregmagolan]

Although, its would be easy to opt-out with an attribute in the macro. I'll add that.

[gregmagolan]

Added

        generate_junit_xml: Add a custom reporter to output junit XML to `process.env.XML_OUTPUT_FILE` where Bazel expects to find it.

            When enabled, `jasmine-reporters` must be linked to the supplied node_modules tree.

that defaults to True

[gregmagolan]

We're removing all the vendoring the same as rules_webpack. Its a much better pattern to have users supply their npm deps with these rule sets.

[jbedard]

Yeah, I know removing it is the goal. I was just mentioning how previously if the user didn't care about reporting they didn't have to do anything, now (at least before adding generate_junit_xml) they would have to install a package they don't even want.

[gregmagolan]

This is definitely a BREAKING CHANGES for users just like the webpack change but this is pre-1.0 so fair game on a minor release. Long term there will be less maintenance here (and less complexity) since there is no versions.bzl to update with new versions. This also make running tools more similar to how tools are run outside of Bazel which is good and doesn't force users to have to keep versions aligned between their package.json & WORKSPACE file.

[jbedard]

I don't care about the breakage, I want a good API. Forcing users to install an npm package they don't even know or care about isn't a great API. I think the generate_junit_xml solves that fine though 👍

[jbedard]

@jbedard the behind the scenes package_json attribute doesn't seem possible with this pattern but maybe the problem goes away entirely when switching to use the user's jasmine?

The issue is the users package.json and the type not aligning with the type junit_reporter.js requires. So I think the bug will be re-introduced here? Although no tests are failing... 🤔

@dgp1130 found + fixed this one... any thought's on this?

[jbedard]

I think jasmine/private/package.json can be deleted now? Might depend on that package_json issue though.

[gregmagolan]

jasmine/private/package.json

Roger. Will delete it for now.

[jbedard]

Can we try reproducing 5cbd285? Maybe clone the examples/commonjs test and put "type": "module" in a package.json file of the example?

[jbedard]

I think renaming junit_reporter.js to junit_reporter.cjs might also solve it. I'm not sure how far back support goes for that in node though.

[gregmagolan]

I think renaming junit_reporter.js to junit_reporter.cjs might also solve it. I'm not sure how far back support goes for that in node though.

Ahh. Yes, I may have hit the same issue then and the rename fixed it. I'll rename the files as well. Forgot to do that.

[dgp1130]

If all the files are named *.mjs or *.cjs then I think it's safe for ESM since Node won't even look for a package.json in that situation. However I think we'd still want a package.json to avoid pulling in the user's exports, imports or other properties which might affect @aspect_rules_jasmine JS code. Otherwise Node will find the user's root package.json and assume anything in there also applies to @aspect_rules_jasmine JS code and could break things.

[gregmagolan]

If all the files are named *.mjs or *.cjs then I think it's safe for ESM since Node won't even look for a package.json in that situation. However I think we'd still want a package.json to avoid pulling in the user's exports, imports or other properties which might affect @aspect_rules_jasmine JS code. Otherwise Node will find the user's root package.json and assume anything in there also applies to @aspect_rules_jasmine JS code and could break things.

I see. That makes sense. There is only the two .cjs files in rules_jasmine jasmine/private/runner.cjs and jasmine/private/junit_reporter.cjs so sounds like we're good on that front then?

[jbedard]

I think what @dgp1130 mentioned might be a concern? Now that the files are running in a directory essentially owned by the user they will be effected by things like a package.json or node_modules?

Can we put them in a subdirectory that contains the empty package.json? I'm not certain that is required though.

[dgp1130]

I imagine we're probably fine. The *.cjs files mitigate any problematic effects from "type": "module". imports would only cause an issue if @aspect_rules_jasmine code imported via something like #runner and exports would only be an issue if it actually exported anything (or re-imported its own export). Since we're not doing any of that, and likely won't any time soon, I imagine this is fine.

There's always the risk of other package.json properties now or in the future introducing incompatibility. For example, if Node were to add "type": "only-use-modules-and-throw-on-any-commonjs-code", then any users with that option would inadvertently break @aspect_rules_jasmine. But it's probably better to just not worry about it for now and deal with it if and when that comes up.

[jbedard]

Thanks for the info and suggestions @dgp1130 👍