Can we drop builtin AMD support?

NullVoxPopuli commented 9 months ago

[!TIP] You can continue to load source and test files with AMD in QUnit 3.0. Refer to: https://qunitjs.com/api/config/autostart/#loading-with-requirejs

AMD is an implementation detail of a bundler, and it feels goofy to have built in.

all tools these days know how to work with The Platform, so I think this could be a good opportunity to have less to maintain.

Krinkle commented 8 months ago

I don't think of QUnit as a library one is meant to bundle. Most test runners afaik share that sentiment, in that test runner generally act on your behalf to run your tests, and your tests import your application code (possibly built/compiled), and the test framework is either imported by your test files (which would not be compiled or built) or ahead of time by the test runner itself.

In the case of AMD, there's usually a top-level file for the app and for the test, where test.html file would first load QUnit and their AMD loader, and then load their tests/application code from there.

I worry that requiring them to have a separate build just for QUnit would lower dev experience, and potentially decrease confidence in the test result as it means they would no longer integrate with their main build (or have a separate build that contains only QUnit, hence why we added upstream support at some point).

The projects I'm aware of that use QUnit and AMD, would, I suspect not benefit from this change. Do you agree?

I am open to dropping native support, but perhaps not for the same reasons as you.

For example, if it becomes a burden to support we could instead recommend that projects build their own qunit.amd.js file. It seems likely that such project might actually not be building any other AMD files yet. For example https://github.com/kiwix/kiwix-js/tree/3.9.0/, used AMD to directly load all test and production source files. (No bundling, it's an offline web app.) They'd have to build a variant of QUnit just to load their tests? Or do you propose we merely remove this from the src but still create an AMD variant during our own release process?

It looks like for ESM we'll need a separate distribution indeed since it's hard to import CJS directly in ESM, and transform services seem to currenlty misunderstand our exports (per https://github.com/qunitjs/qunit/issues/1724), so providing our own one would make that work directly, possibly even without needig to enumerate each export by name (we have quite a few).

For AMD, it seems like it'd be trivial to continue support in the non-ESM ("CJS") distribution with these three lines of code as-is.

NullVoxPopuli commented 8 months ago

I worry that requiring them to have a separate build just for QUnit would lower dev experience

Given that this works: https://jsbin.com/fipayiy/edit?html,output, I don't think we need to support any target format other than ESM.

if it becomes a burden to support we could instead recommend that projects build their own

If folks are still using AMD without a tool to build AMD for them, then I think a wrapper script / build would be fine. The main thing I want to get away from is the single file supporting every format in existence.

Or do you propose we merely remove this from the src but still create an AMD variant during our own release process?

yeah, I am ok with this. I'm a big fan of:

author only in one format
build to all supported formats
test all supported formats in an isolated way via monorepo (which gives us the most realistic way to reference our built project)

it's hard to import CJS directly in ESM, and transform services

this happens in tool-less situations (local browser) as well. transform services are irrelevant.

For AMD, it seems like it'd be trivial to continue support in the non-ESM ("CJS") distribution with these three lines of code as-is.

yeah, and if we need to bundle an IIFE format, AMD is IIFE + the 3 lines easy peasy.

NullVoxPopuli commented 8 months ago

so, I think the main thing I'd like to do organizationally is move the repo to a monorepo so we can have a setup like:

./<qunit> (existing files)
./test-packages/
  ./browser/
    ./amd/
    ./esm/
    ./script-import-map/
    ./bundled-esm/
    ...
  ./node
    ./esm/
    ./cjs/
    ...

as far as I know, only pnpm supports this type of monorepo where the top level is also a publishable package. thoughts?

NullVoxPopuli commented 8 months ago

@Krinkle is there a discord or some other chat platform where we'd be able to talk more synchronously about planning the future of the repo?

Krinkle commented 8 months ago

@NullVoxPopuli Yes, we have a Matrix room at https://app.gitter.im/#/room/#qunitjs_qunit:gitter.im (webchat), or via other clients: https://matrix.to/#/#qunitjs_qunit:gitter.im

Krinkle commented 4 weeks ago

I've done a bit more research as part of https://github.com/qunitjs/qunit/issues/1551 (ESM support) which is somewhat orthogonal to this. Apologies if you know this already, I'm mainly sharing here for future reference and for transparancy. (Not specifically to NullVoxPopuli).

Findings

Our AMD export sets QUnit.config.autostart = false as side-effect.
Our AMD export sometimes prevents globalThis.QUnit from being set.

Action

Remove AMD export. No longer needed by AMD users.
Ensure globalThis.QUnit is always set consistency.
Remove QUnit.config.autostart = false setting. No longer needed by AMD users.

Background: AMD export

The feature we document and support is disabling autostart to load test files asynchronously. It doesn't matter to QUnit how you load your test files. It used to be that the most common reason someone would have to disable autostart, was to use AMD/RequireJS. Nowadays, a more likely reason is ESM async import(). Either way requires the same feature, and I've updated the docs to lead with an ESM import() example at https://qunitjs.com/api/config/autostart/.

Loading QUnit itself has not been recommended, documented, or tested for quite some time.

Our own autostart docs, and upstream RequireJS recommend to first loaded qunit.js normally, and then used AMD for individual test files.

As such, we simply don't need it. This doesn't affect the long tail of projects (such as Kiwix, mentioned above) that use RequireJS to load their code and/or their tests.

Background: Automatic config.autostart side-effect

I'd like to make it safe in QUnit 3.0 to load qunit.js multiple times. Today we consider this an error and throw an error. To adopt ESM we will likely have to provide a separate build. For projects that reference QUnit globally, with one part of the test stack responsible for actually loading it, this will work fine. They can switch or not switch depending on their needs and requirements, it's fine either way.

For projects that reference QUnit by importing it in individual plugins, integration layers, and/or test files, there's a good chance that there will be mixed use in practice. If we don't do anything to prevent it, this will cause various split-brain problems where config/plugins are missing, or tests are divided, event handlers firing multiple times, etc. This also came up in discussions for jQuery at https://github.com/jquery/jquery/pull/5429/ where similar concerns were raised. For QUnit, we're a bit more flexible in some ways and less flexible in others.

I'm thinking of making QUnit safe to load multiple times. We're not concerned about clashing versions of QUnit. These are going to be the same version. Whichever loads first will own the state, and subsequent imports (e.g. CJS first then ESM, or vice versa) will effectively resume that instead of exporting a different local copy.

In refactoring the code, I stubmled upon this:

https://github.com/qunitjs/qunit/blob/1fc486503499be4280d3362f3c5e866e6115cd8f/src/export.js#L36-L40

We could grandfather this in, by replacing it with a change to src/config.js where we can set the autostart default to set based on if define.amd exists. But..., do we actually need this to support AMD?

From what I can tell..., no, we don't! Our own autostart docs, and upstream RequireJS docs already set this explicitly. Anecdotally, Kiwix does as well (example). I suggest we simply remove this in QUnit 3.0.

qunitjs / qunit