Skip to content

Skip happy-dom registration in browsers#121

Open
DmitrySharabin wants to merge 2 commits into
rollback-signalsfrom
test-setup-browser-compat
Open

Skip happy-dom registration in browsers#121
DmitrySharabin wants to merge 2 commits into
rollback-signalsfrom
test-setup-browser-compat

Conversation

@DmitrySharabin
Copy link
Copy Markdown
Member

@DmitrySharabin DmitrySharabin commented May 16, 2026

Conditionally register happy-dom in Node only so the test entry can also load in a browser. Restructure test/index.js to await happy-dom before importing test modules, avoiding a TLA-vs-sync-sibling race on PropChangeEvent's CustomEvent base.

🤖 Generated with Claude Code

@DmitrySharabin DmitrySharabin requested a review from LeaVerou May 16, 2026 13:35
Copy link
Copy Markdown
Contributor

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of turning all our imports into async imports, which doesn't scale, we can just import (statically) a file which checks if we have a DOM and if not, dynamic imports happy-dom and does the other stuff. Then all the complexity lives in that file, which is imported normally before other imports in every test that needs it. What am I missing?

DmitrySharabin and others added 2 commits May 17, 2026 22:13
Gate `@happy-dom/global-registrator` import + register on
`typeof HTMLElement === "undefined"` so the setup is a no-op in browsers.
Restructure test/index.js to await happy-dom dynamically before importing
test modules, avoiding a TLA-vs-sync-sibling race on PropChangeEvent's
CustomEvent base class.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
test/index.js becomes a thin boot wrapper that statically imports
test/util/happy-dom.js (so its TLA completes before the body runs),
then dynamic-imports test/index-fn.js (which holds the plain static
imports of the test tree) and restores native CustomEvent.

Matches the src/index.js ↔ src/index-fn.js convention and addresses
Lea's review on #121: complexity stays in one place, the test tree
uses plain static imports again.

Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]>
@DmitrySharabin DmitrySharabin force-pushed the test-setup-browser-compat branch from 6ba8544 to f9e3a29 Compare May 18, 2026 08:53
@DmitrySharabin
Copy link
Copy Markdown
Member Author

Tried that first (it was actually our initial shape). The literal version — test/index.js keeps static imports, the conditional await import("@happy-dom/global-registrator") lives inside test/util/happy-dom.js — fails on Node 24 with Failed to execute 'dispatchEvent' on 'EventTarget': parameter 1 is not of type 'Event'. ESM lets sibling modules evaluate while a TLA-holding sibling is paused, so PropChangeEvent (loaded transitively via Prop.js) does extends CustomEvent before happy-dom registers and ends up extending native CustomEvent.

f9e3a29 keeps the spirit of your suggestion: test/index.js is a thin boot wrapper that statically imports happy-dom.js (whose TLA resolves before the body runs), then dynamic-imports test/index-fn.js which holds plain static imports of the test tree. Same index.js/index-fn.js convention as src/.

— answered by Claude (Opus 4.7) on behalf of @DmitrySharabin

@DmitrySharabin DmitrySharabin requested a review from LeaVerou May 18, 2026 08:55
Copy link
Copy Markdown
Contributor

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, tests need to be able to run independently, they shouldn't break if not loaded from the root.

Until we find a better solution I'd suggest loading it in every case and just not registering in browser environments. It's wasteful, but at least the wart then lives with that file, rather than infecting the entire testsuite.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants