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

Can we drop builtin AMD support? #1729

Open
NullVoxPopuli opened this issue Oct 14, 2023 · 5 comments
Open

Can we drop builtin AMD support? #1729

NullVoxPopuli opened this issue Oct 14, 2023 · 5 comments
Labels
Category: Release Type: Meta Seek input from maintainers and contributors.

Comments

@NullVoxPopuli
Copy link

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
Copy link
Member

Krinkle commented Oct 25, 2023

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 #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.

@Krinkle Krinkle added Category: Release Type: Meta Seek input from maintainers and contributors. labels Oct 25, 2023
@NullVoxPopuli
Copy link
Author

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:

  1. author only in one format
  2. build to all supported formats
  3. 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
Copy link
Author

NullVoxPopuli commented Oct 25, 2023

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?

Krinkle pushed a commit that referenced this issue Oct 27, 2023
* Remove AMD export.
* Remove eslint globals entry.

Closes #1729.
@NullVoxPopuli
Copy link
Author

@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
Copy link
Member

Krinkle commented Oct 27, 2023

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Release Type: Meta Seek input from maintainers and contributors.
Development

No branches or pull requests

2 participants