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

ESM build #1551

Closed
matthewp opened this issue Jan 28, 2021 · 9 comments · Fixed by #1798
Closed

ESM build #1551

matthewp opened this issue Jan 28, 2021 · 9 comments · Fixed by #1798
Labels
Category: Release Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Milestone

Comments

@matthewp
Copy link

An ESM build would be nice given that browser support for modules is mature these days.

Currently to use modules when testing with QUnit I do something like this:

<script src="https://code.jquery.com/qunit/qunit-VERSION.js"></script>
<script type="module" src="./test.js"></script>

local_qunit.js

export default window.QUnit;

test.js

import QUnit from './local_qunit.js';

Given that qunit is using rollup for the build it should be relatively straight-foward to add a second build output for use with <script type="module"> so you would be able to do:

import QUnit from 'https://code.jquery.com/qunit/qunit-VERSION.mjs';

...
@Krinkle
Copy link
Member

Krinkle commented Jan 30, 2021

@matthewp There shouldn't be any need to bring the QUnit object in as a module import. It is meant to bootstrap the HTML page for you and takes care of itself. The test suites should reference QUnit API as a host variable, similar to how you might interact with window, location, URL etc.

Could you explain what problem or advantage you are currently addressing by using QUnit in this way? I'd prefer not to maintain multiple build outputs, but I could consider it or come up with simpler solutions, if I understood the use case better. Thanks!

@Krinkle Krinkle added Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors. Category: Release labels Jan 30, 2021
@matthewp
Copy link
Author

Maybe I'm not following but I need to reference the QUnit object to do:

QUnit.test('my test')....

The advantage to it being available as a module is the same as for any module.

@Krinkle
Copy link
Member

Krinkle commented Jan 31, 2021

@matthewp The QUnit object is always available. Does it not work from your ESM-loaded test.js file? There is no need to import qunit a second time from inside every test suite file.

@reno1979
Copy link

Same problem here.

Test modules that require QUnit al import QUnit, and I need to build them before the can be used.

@matthewp
Copy link
Author

@Krinkle Yeah maybe I wasn't being clear about my reasoning here. Yes, QUnit works fine as a global via a classic script tag. There's nothing that doesn't work about it. It's just that I would rather my modules explicitly import their dependencies and not rely on external dependency management (having to remember to always include a script tag in the right order on the page). So the reason to have QUnit be a module is the same reason to have any dependency to be a module.

@jdittrich
Copy link

It's just that I would rather my modules explicitly import their dependencies and not rely on external
dependency management (having to remember to always include a script tag in the right order on the page).

Same for me: Having explicit imports makes code easier to reason about for me: When I read the tests, I want to know where objects come from. Currently I need to know that QUnit comes as a global when I run my tests in a browser window.

@Krinkle Krinkle added this to the 3.0 release milestone Jun 18, 2024
@Krinkle
Copy link
Member

Krinkle commented Jun 18, 2024

This will also resolve #1724 once done.

Krinkle added a commit that referenced this issue Jun 18, 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.
Except in in the QUnit CLI we still set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js. So
really the only case where the QUnit global is missing is:

* AMD context, where QUnit would be a local argument name in the
  define/require() callback in each file.
* Custom test runners that 1) run in Node.js, and 2) require qunit
  directly and 3) don't also export it as a global.

I'm aware a growing proportion of users import 'qunit' directly in
each test file, to aid TypeScript for example. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially in a browser context with
a simple no-build-step project.

== Why ==

1. Improve portability between test runners.

Make it QUnit's responsiblity to define this reliably, as indeed
it almost always already does, instead of requiring specific test
runners to patch this up on their end.

Esp 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 those.

2. 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 around test registry
and other state, standardise internally on whichever globalThis.QUnit
was defined first, and then reliably export that to any importers,
regardless of import method.

Ref #1551.
@Krinkle
Copy link
Member

Krinkle commented Jun 22, 2024

Quoting here to ease discovery.

From #1729 (comment):

[…]

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

[…]

Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 22, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
qunitjs#1118 and
qunitjs#1711.

Ref qunitjs#1486.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at qunitjs#1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit that referenced this issue Jun 23, 2024
=== What ===

Turn html.js into an HtmlReporter class where the HtmlReporter.init
static method contains (most) of the browser runner logic that we
would run even if the HtmlReporter were turned off.

This patch primarily deals with making html.js itself free of
side-effects by delaying any browser setup logic to the init()
function.

In a future patch we can take this further and move the logic to a
"browser runner" function of sorts, such that what is left in the
reporter can be made conditional on the presence of `div[id=qunit]`
and/or `QUnit.config.reporters.html`. This is tracked in
#1118 and
#1711.

Ref #1486.

Intentional changes:

* Use of previousFailure is no longer conditional on QUnit.config.reorder
  in order to make it more stateless. The message already did not state
  that it relates to config.reorder, and stating that it previously failed
  is accurate either way.

* Calling ev.preventDefault() is no longer conditional.
  This has been supported since IE9. QUnit 2 already required IE9+,
  and QUnit 3.0 will require IE11.
  jQuery removed similar check in jQuery 2.2.0, with commit
  jquery/jquery@a873558436.

=== Why ===

We need html.js to be side-effect free in order to add native ESM
support, tracked at #1551.

To support native ESM exports, we need to introduce a separate
distribution file since ESM export syntax isn't allowed in scripts.
I expect numerous projects to involve mixed ESM imports and CJS require
usage, especially when integration layers and plugins are involved.

To avoid a split-brain problem, this means we need to somehow have
both export the same one. The way I intend to do this is by detecting
if QUnit was already defined, and if so, discard our local definition
and use that one instead. This means we need to only init the browser
runner only if we're the first one (e.g. window.onerror, QUnit.begin
for fixture, QUnit.testDone for HTML reporting, etc).

To accomplish that, start by making html.js side-effect free, deferring
its init code to a (for now) unconditional call at the end of qunit.js.
Krinkle added a commit that referenced this issue Jun 23, 2024
This refactors the initFixture and initUrlconfig code to be an exported
callable so that it is safe to define without side-effects and can then
be called conditionally.

The previous code was already conditionally with an inline check
for window/document. This is now centralised in prep for making it
further conditional on whether or not QUnit was already exported,
thus making it safe to load QUnit twice (e.g. ESM and CJS without
split-brain conflict). Tracked at
#1551.

Follows-up e1e03e6.

Closes #1118.
Krinkle added a commit that referenced this issue Jun 23, 2024
This refactors the initFixture and initUrlconfig code to be an exported
callable so that it is safe to define without side-effects and can then
be called conditionally.

The previous code was already conditionally with an inline check
for window/document. This is now centralised in prep for making it
further conditional on whether or not QUnit was already exported,
thus making it safe to load QUnit twice (e.g. ESM and CJS without
split-brain conflict). Tracked at
#1551.

Follows-up e1e03e6.

Closes #1118.
Krinkle added a commit to Krinkle/qunit that referenced this issue 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.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

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

2. 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 qunitjs#1551.
Krinkle added a commit to Krinkle/qunit that referenced this issue 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.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

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

2. 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 qunitjs#1551.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
qunitjs#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit to Krinkle/qunit that referenced this issue 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.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

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

2. 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 qunitjs#1551.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
qunitjs#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit to Krinkle/qunit that referenced this issue 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.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

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

2. 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 qunitjs#1551.
Krinkle added a commit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit that referenced this issue Jun 24, 2024
In particular, events.js was the only test we had in which a QUnit
test was loading another QUnit instance. This fails as-is after the
"Always define globalThis.QUnit" patch for
#1551, because it would return
the instance that's already running instead of a separate one.

It will still be possible to do this, by adding `delete global.QUnit`
before the import, but that's not the purpose of the events test.
Instead, I've added a separate test that proves inception works.
Krinkle added a commit to Krinkle/qunit that referenced this issue 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.
Except in the QUnit CLI, where we do set the global anyway, because
that's how most QUnit tests are written, incliuding for Node.js.
So really the only case where the QUnit global is missing is custom
test runners that:

* run in Node.js,
* and require/import qunit.js directly,
* and don't export it as a global.

I'm aware a growing proportion of developers import 'qunit' directly
in each test file for improved type support. That's a great way
to avoid needing to rely on globals. But, it's not a requirement,
and is not always an option, especially for simple no-build-step
and browser-facing projects.

== Why ==

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

2. 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 qunitjs#1551.
Krinkle added a commit that referenced this issue Jul 6, 2024
* Add explicit file extensions as per ESM standard, instead of relying
  on Node.js-specific require() resolution, or Rollup-specific ESM
  resolution for Node.js compat.

* As minor prep for external ESM support, move UMD export to
  the /src/qunit.js entrypoint. I considered adding an `export`
  statement to the entrypoint, but this interfers with generating
  the CJS distribution with Rollup the way we do today.

  - Adding `export default` to /src/qunit.js, creates a Rollup error
    about  `output: none` for input files that perform exports.
  - Setting `output: defaults` in Rollup config, creates a warning
    about having an export while using `format: iife` but having set
    no name for it.
  - Adding a name would change the output in a meaningful way,
    namely it would create an (implied global) variable in the form
    of `var QUnit = (function () { … return QUnit; }());`
  - Creating such variable using `var` instead of as `window.QUnit`
    means that it cannot be unset via `delete window.QUnit` which
    breaks certain test, and is out of the scope for this pathc.

* Fix fragile code in stracktrace.js that previously worked only because
  of Babel transformations masking a violation of the Temporal Dead Zone
  between `const fileName` and the functions it uses to compute that
  value.

```
$ node --experimental-detect-module
Welcome to Node.js v21.1.0.
Type ".help" for more information.
> await import("./src/qunit.js");
Uncaught ReferenceError: Cannot access 'fileName' before initialization
    at extractStacktrace (file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:51:5)
    at sourceFromStacktrace (file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:81:10)
    at file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:35:19
    at …
    at async REPL
```

After this:

```
> await import("./src/qunit.js");
//> null
> (await import("./src/core.js")).default;
//> { version: …, module: …, test: … }
```

Ref #1551.
Krinkle added a commit that referenced this issue Jul 6, 2024
* Add explicit file extensions as per ESM standard, instead of relying
  on Node.js-specific require() resolution, or Rollup-specific ESM
  resolution for Node.js compat.

* As minor prep for external ESM support, move UMD export to
  the /src/qunit.js entrypoint. I considered adding an `export`
  statement to the entrypoint, but this interfers with generating
  the CJS distribution with Rollup the way we do today.

  - Adding `export default` to /src/qunit.js, creates a Rollup error
    about  `output: none` for input files that perform exports.
  - Setting `output: defaults` in Rollup config, creates a warning
    about having an export while using `format: iife` but having set
    no name for it.
  - Adding a name would change the output in a meaningful way,
    namely it would create an (implied global) variable in the form
    of `var QUnit = (function () { … return QUnit; }());`
  - Creating such variable using `var` instead of as `window.QUnit`
    means that it cannot be unset via `delete window.QUnit` which
    breaks certain test, and is out of the scope for this pathc.

* Fix fragile code in stracktrace.js that previously worked only because
  of Babel transformations masking a violation of the Temporal Dead Zone
  between `const fileName` and the functions it uses to compute that
  value.

```
$ node --experimental-detect-module
Welcome to Node.js v21.1.0.
Type ".help" for more information.
> await import("./src/qunit.js");
Uncaught ReferenceError: Cannot access 'fileName' before initialization
    at extractStacktrace (file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:51:5)
    at sourceFromStacktrace (file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:81:10)
    at file:///Users/krinkle/Development/qunit/src/core/stacktrace.js:35:19
    at …
    at async REPL
```

After this:

```
> await import("./src/qunit.js");
//> null
> (await import("./src/core.js")).default;
//> { version: …, module: …, test: … }
```

Ref #1551.
Krinkle added a commit that referenced this issue Jul 15, 2024
Remove any remaining dynamic or indirection in declaring the exports.

Ref #1551.
Ref #1724.
Krinkle added a commit that referenced this issue Jul 15, 2024
Remove any remaining dynamic or indirection in declaring the exports.

Ref #1551.
Ref #1724.
@Krinkle
Copy link
Member

Krinkle commented Jul 20, 2024

Summary of recent changes, mostly around getting core.js into a shape where it "just" exports stuff simply and straight-forwardly, in a way we can convert and expose as ESM, and easily explain/document as such:

  • Core: Export QUnit.urlParams unconditionally. 57c2dbc
  • HTML Reporter: Refactor as class without side-effects, export HtmlReporter as QUnit.reporters.html. e1e03e6
  • Browser Runner: Refactor to side-effect free file, simply call conditionally in the entry point file. 6a8318a
  • Test: Remove reliance on subtle export/require details. 57a543b
  • Core: Remove reliance on Rollup's extension-less imports. Make src/ compatible with ESM in Node.js as-is. 95105aa
  • Core: Reduce src/core.js to a single object literal. No more indirect assignments or extensions. d395525

Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 24, 2024
In 05e15ba, I converted QUnit.start() to be generated by
createStartFunction(QUnit) to avoid a circular dependency.

This doesn't work in practice with ESM. See inline comment added
for details.

Ref qunitjs#1551.
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 24, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 25, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 25, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 25, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 25, 2024
Krinkle added a commit to Krinkle/qunit that referenced this issue Jul 29, 2024
Krinkle added a commit that referenced this issue Aug 25, 2024
Follows-up 05e15ba, which made this into a factory function,
but that has the downside of making the QUnit object not defined in
one object literal, which makes a few other things reasier to reason
about.

In a way, it's more honest to say that start is the product of a
factory function, but I'd prefer to maintain the simplicity of an
uncoupled literal declaration the entire API, in particular in prep
for native ESM export (ref #1551).

I'll accept in return the internal responsiblity to not call start()
"incorrectly" (i.e. before it is ready). This responsibility does not
leak into, complicate, break, or otherwise change the public API, and
is mitigated by a runtime detection, for the benefit of future
contributors and maintainers to QUnit.
Krinkle added a commit that referenced this issue Aug 26, 2024
Follows-up 05e15ba, which made this into a factory function,
but that has the downside of making the QUnit object not defined in
one object literal, which makes a few other things reasier to reason
about.

In a way, it's more honest to say that start is the product of a
factory function, but I'd prefer to maintain the simplicity of an
uncoupled literal declaration the entire API, in particular in prep
for native ESM export (ref #1551).

I'll accept in return the internal responsiblity to not call start()
"incorrectly" (i.e. before it is ready). This responsibility does not
leak into, complicate, break, or otherwise change the public API, and
is mitigated by a runtime detection, for the benefit of future
contributors and maintainers to QUnit.
@Krinkle Krinkle closed this as completed in ec7d6e7 Sep 4, 2024
Krinkle added a commit to jquery/codeorigin.jquery.com that referenced this issue Nov 4, 2024
Krinkle added a commit to jquery/codeorigin.jquery.com that referenced this issue Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Release Type: Enhancement New idea or feature request. Type: Meta Seek input from maintainers and contributors.
Development

Successfully merging a pull request may close this issue.

4 participants