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

Core: Always define globalThis.QUnit #1771

Merged
merged 1 commit into from
Jul 30, 2024
Merged

Conversation

Krinkle
Copy link
Member

@Krinkle Krinkle commented Jun 24, 2024

Background

In QUnit 2.x, we always set the QUnit global in browser contexts, but
outside browsers, we only set the global if CJS/AMD wasn't detected.
Then, in the QUnit CLI, we set the global anyway, and we expect other
test runners to do the same, since that's how most QUnit tests are
written, including for Node.js targets.

In practice, the QUnit global would only be missing in custom test
runners that 1) target Node.js, 2) require/import qunit.js directly,
and 3) don't export it as a global.

I'm aware many communities import 'qunit' directly in each test file
for improved type support. That's great, and avoids needing to rely
on globals. But, that's not a requirement I want to impose on everyone,
especially for simple no-build-step and browser-facing projects.

Why

Improve portability between test runners.
Remove the last edge case where the QUnit global can be undefined.
Make it QUnit's responsiblity to define this reliably, as indeed it
almost always already does. Remove this as undocumented requirement
for specific test runners to patch up on their end.

In light of Karma deprecation, and emergence of more general purpose
TAP runners, I'd like to remove this as factor that might make/break
QUnit support in one of those.


Previous version of this PR:

Prepare for native ESM build.
We don't natively support ESM exports yet, but once we do this will become a problem. To prevent split-brain problems with mixed use (e.g. in test registry and other state) standardise internally on which ever globalThis.QUnit was defined first, and then reliably export that to any importers.
Ref #1551.

@Krinkle Krinkle requested review from timmywil, gibson042 and mgol June 24, 2024 04:24
@Krinkle Krinkle added this to the 3.0 release milestone Jun 24, 2024
@Krinkle Krinkle marked this pull request as draft July 9, 2024 22:19
@Krinkle
Copy link
Member Author

Krinkle commented Jul 9, 2024

Per discussion in the Matrix channel, I'm going to ponder on this a bit more. Firstly, I'll add a few more integration demos that utilize import/embed QUnit in different ways.

== Background

In QUnit 2.x, we always set the QUnit global in browser contexts, but
outside browsers, we only set the global if CJS/AMD wasn't detected.
Then, in the QUnit CLI, we set the global anyway, and we expect other
test runners to do the same, since that's how most QUnit tests are
written, including for Node.js targets.

In practice, the QUnit global would only be missing in custom test
runners that 1) target Node.js, 2) require/import qunit.js directly,
and 3) don't export it as a global.

I'm aware many communities import 'qunit' directly in each test file
for improved type support. That's great, and avoids needing to rely
on globals. But, that's not a requirement I want to impose on everyone,
especially for simple no-build-step and browser-facing projects.

== Why

* Improve portability between test runners.
  Remove the last edge case where the QUnit global can be undefined.
  Make it QUnit's responsiblity to define this reliably, as indeed it
  almost always already does. Remove this as undocumented requirement
  for specific test runners to patch up on their end.

  In light of Karma deprecation, and emergence of more general purpose
  TAP runners, I'd like to remove this as factor that might make/break
  QUnit support in one of those.
@Krinkle
Copy link
Member Author

Krinkle commented Jul 30, 2024

I've reduced and rescoped this change only to ensuring a QUnit global exists. The refactoring, and the removal of the load-once restriction, for ESM prep, I've reverted back out. Per the feedback from @mgol and @timmywil I'm adding a bunch of demos and integration scenarios first, at #1787. I'll then probably take a similar approach to jQuery core, and see if that still pasess under the same conditions and scenarios in that PR.

@Krinkle Krinkle marked this pull request as ready for review July 30, 2024 16:52
@Krinkle Krinkle merged commit 5bff13a into qunitjs:main Jul 30, 2024
10 checks passed
@Krinkle Krinkle deleted the esm-prep branch July 30, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

1 participant