feat: allow passing jsdom to new LynxTestingEnv#1422
feat: allow passing jsdom to new LynxTestingEnv#1422upupming merged 6 commits intolynx-family:mainfrom
Conversation
🦋 Changeset detectedLatest commit: e72ee8d The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughLynxTestingEnv now accepts an optional JSDOM instance via its constructor; code and docs were updated to pass a JSDOM instance explicitly, a generated API report file was removed, and the vitest setup now injects the JSDOM instance instead of setting Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.changeset/rare-mice-leave.md(1 hunks)packages/testing-library/testing-environment/README.md(1 hunks)packages/testing-library/testing-environment/etc/test-environment.api.md(0 hunks)packages/testing-library/testing-environment/etc/testing-environment.api.md(1 hunks)packages/testing-library/testing-environment/src/env/vitest/index.ts(2 hunks)packages/testing-library/testing-environment/src/index.ts(4 hunks)
💤 Files with no reviewable changes (1)
- packages/testing-library/testing-environment/etc/test-environment.api.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
📚 Learning: in the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:23:07.797Z
Learning: In the lynx-family/lynx-stack repository, changesets are only required for meaningful changes to end-users such as bugfixes and features. Internal/development changes like chores, refactoring, or removing debug info do not need changeset entries.
Applied to files:
.changeset/rare-mice-leave.mdpackages/testing-library/testing-environment/src/index.ts
📚 Learning: in the lynx-family/lynx-stack repository, ci checks require changesets when files matching the patte...
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1330
File: .changeset/olive-animals-attend.md:1-3
Timestamp: 2025-07-22T09:26:16.722Z
Learning: In the lynx-family/lynx-stack repository, CI checks require changesets when files matching the pattern "src/**" are modified (as configured in .changeset/config.json). For internal changes that don't need meaningful changesets, an empty changeset file is used to satisfy the CI requirement while not generating any release notes.
Applied to files:
.changeset/rare-mice-leave.md
📚 Learning: in the lynx-stack ssr implementation, each createlynxview instance is used to render once and then d...
Learnt from: PupilTong
PR: lynx-family/lynx-stack#1029
File: packages/web-platform/web-core-server/src/createLynxView.ts:0-0
Timestamp: 2025-07-16T06:26:22.230Z
Learning: In the lynx-stack SSR implementation, each createLynxView instance is used to render once and then discarded. There's no reuse of the same instance for multiple renders, so event arrays and other state don't need to be cleared between renders.
Applied to files:
packages/testing-library/testing-environment/src/index.ts
🔇 Additional comments (9)
.changeset/rare-mice-leave.md (1)
1-6: LGTM! Changeset properly documents the feature.The changeset correctly identifies this as a patch-level change and accurately describes the fix for the jsdom initialization issue. The description aligns well with the code changes that make jsdom dependency explicit.
packages/testing-library/testing-environment/README.md (2)
11-11: LGTM! Import addition is correct.The JSDOM import is properly added to support the updated usage example.
13-13: LGTM! Constructor usage updated correctly.The example now properly demonstrates passing a JSDOM instance to the LynxTestingEnv constructor, which matches the updated API signature.
packages/testing-library/testing-environment/etc/testing-environment.api.md (1)
91-91: LGTM! API documentation correctly reflects constructor changes.The API report properly documents the new optional
jsdomparameter in theLynxTestingEnvconstructor, which aligns with the implementation changes.packages/testing-library/testing-environment/src/env/vitest/index.ts (2)
3-3: LGTM! Import addition supports explicit jsdom passing.The JSDOM import is correctly added to enable explicit passing to the LynxTestingEnv constructor.
14-14: LGTM! Improved jsdom initialization pattern.The explicit passing of
fakeGlobal.jsdomto the LynxTestingEnv constructor is cleaner and more reliable than the previous global assignment approach. This eliminates potential initialization issues whenglobal.jsdomis undefined.packages/testing-library/testing-environment/src/index.ts (3)
411-411: LGTM! JSDoc example updated correctly.The example properly demonstrates the new constructor usage with explicit JSDOM instantiation.
433-433: LGTM! JSDoc example updated correctly.The background thread example properly shows the new constructor pattern.
449-449: LGTM! JSDoc example updated correctly.The main thread example properly demonstrates the updated constructor usage.
CodSpeed Performance ReportMerging #1422 will not alter performanceComparing Summary
|
Web Explorer#4254 Bundle Size — 348.12KiB (0%).e72ee8d(current) vs 7ccf12f main#4252(baseline) Bundle metrics
|
| Current #4254 |
Baseline #4252 |
|
|---|---|---|
 Initial JS |
143.37KiB |
143.37KiB |
| Initial CSS |
31.84KiB |
31.84KiB |
| Cache Invalidation |
0% |
0% |
| Chunks |
7 |
7 |
| Assets |
7 |
7 |
 Modules |
212(+0.47%) |
211 |
| Duplicate Modules |
17 |
17 |
| Duplicate Code |
3.93%(-0.25%) |
3.94% |
| Packages |
4 |
4 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4254 |
Baseline #4252 |
|
|---|---|---|
| JS |
233.32KiB |
233.32KiB |
| Other |
82.95KiB |
82.95KiB |
| CSS |
31.84KiB |
31.84KiB |
Bundle analysis report Branch upupming:feat/testing-env-jsdom Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#4261 Bundle Size — 237.04KiB (0%).e72ee8d(current) vs 7ccf12f main#4259(baseline) Bundle metrics
|
| Current #4261 |
Baseline #4259 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
0% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
158 |
158 |
| Duplicate Modules |
64 |
64 |
| Duplicate Code |
45.86% |
45.86% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #4261 |
Baseline #4259 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
91.28KiB |
91.28KiB |
Bundle analysis report Branch upupming:feat/testing-env-jsdom Project dashboard
Generated by RelativeCI Documentation Report issue
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/testing-library/testing-environment/src/index.ts (1)
458-458: Constructor param precedence over global jsdom is correct.Thanks for flipping the precedence to prefer the explicit constructor arg over the global. This aligns with explicit DI expectations and matches prior feedback.
🧹 Nitpick comments (2)
packages/testing-library/testing-environment/src/index.ts (2)
464-471: Remove redundant console polyfill; it’s immediately overridden by injectors.
consolefromthis.jsdom.windowis assigned here but later replaced bycreatePreconfiguredConsole()in both injectors, making this assignment dead/possibly confusing.Apply this diff to reduce churn and clarify intent:
- console: this.jsdom.window['console'], // `Event` is required by `fireEvent` in `@testing-library/dom` Event: this.jsdom.window.Event, // `window` is required by `getDocument` in `@testing-library/dom` window: this.jsdom.window, // `document` is required by `screen` in `@testing-library/dom` document: this.jsdom.window.document,Optional hardening: if your tests use dispatching of custom events, consider adding
CustomEventalongsideEvent:Event: this.jsdom.window.Event, + CustomEvent: this.jsdom.window.CustomEvent,Note on rstest compatibility: keeping the injector-owned console is consistent with the prior requirement that both target and global console have methods defined after thread switching. This change preserves that behavior.
408-409: Doc examples: add JSDOM import and mention fallback.The examples now show passing a
new JSDOM(). Add the import to make them copy-pasteable, and optionally call out thatglobal.jsdomcan be used when set by the test runner.Example tweak:
import { JSDOM } from 'jsdom'; import { LynxTestingEnv } from '@lynx-js/testing-environment'; const lynxTestingEnv = new LynxTestingEnv(new JSDOM()); // Alternatively, if your test runner sets global.jsdom: new LynxTestingEnv()Also applies to: 430-431, 446-447
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/testing-library/testing-environment/src/index.ts(4 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1453
File: vitest.config.ts:49-61
Timestamp: 2025-08-06T13:28:57.182Z
Learning: In the lynx-family/lynx-stack repository, the file `packages/react/testing-library/src/vitest.config.js` is source code for the testing library that gets exported for users, not a test configuration that should be included in the main vitest projects array.
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/react/testing-library/src/plugins/vitest.ts:59-61
Timestamp: 2025-08-11T06:00:04.376Z
Learning: In the lynx-family/lynx-stack repository, the `testingLibraryPlugin` in `packages/react/testing-library/src/plugins/vitest.ts` intentionally uses `process.exit` when jsdom installation fails, maintaining consistency with the previous implementation from `packages/react/testing-library/src/vitest.config.js`. This behavior should not be changed to use `this.error` despite being a Vite plugin best practice.
📚 Learning: 2025-08-11T05:57:18.212Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Applied to files:
packages/testing-library/testing-environment/src/index.ts
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/testing-library/testing-environment/src/index.ts (1)
38-56: Add optionaljsdomto the global declaration to satisfy TypeScriptNo existing global augmentation for
jsdomwas found.packages/testing-library/testing-environment/src/index.tsalready importsJSDOMand usesglobal.jsdom(e.g.,this.jsdom = jsdom ?? global.jsdom;), so add an optional global declaration.Files to update:
- packages/testing-library/testing-environment/src/index.ts — add to the existing
declare globalblock.- (note) packages/testing-library/testing-environment/src/env/vitest/index.ts references
fakeGlobal.jsdomand deletesglobal.jsdomat teardown; no change required there.Suggested diff:
declare global { var lynxTestingEnv: LynxTestingEnv; var elementTree: ElementTree; var __JS__: boolean; var __LEPUS__: boolean; var __BACKGROUND__: boolean; var __MAIN_THREAD__: boolean; + // Provided by the test runner (e.g., via a setup file). Optional. + var jsdom: JSDOM | undefined; namespace lynxCoreInject { var tt: any; }
🧹 Nitpick comments (5)
packages/testing-library/testing-environment/src/index.ts (5)
456-456: Consider making the injected JSDOM instance immutable
jsdomis a constructor-injected dependency and shouldn’t change at runtime. Marking it as readonly communicates intent and avoids accidental reassignment.- jsdom: JSDOM; + readonly jsdom: JSDOM;
459-459: Prefer globalThis over global for the fallbackMinor consistency and typing improvement: elsewhere the code already leans on
globalThis. Using it here clarifies intent and aligns with the global augmentation pattern.- this.jsdom = jsdom ?? global.jsdom; + this.jsdom = jsdom ?? globalThis.jsdom;
405-412: Doc example: import JSDOM explicitlyThe snippet uses
new JSDOM()but doesn’t show its import, which may confuse readers. Add the import line.* ```ts - * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { JSDOM } from 'jsdom'; * * const lynxTestingEnv = new LynxTestingEnv(new JSDOM()); *
428-436: Doc example: import JSDOM explicitlySame as above; include the import for completeness.
* ```ts - * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { JSDOM } from 'jsdom'; * * const lynxTestingEnv = new LynxTestingEnv(new JSDOM()); *
444-452: Doc example: import JSDOM explicitlyRepeat the import addition for this snippet as well.
* ```ts - * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { LynxTestingEnv } from '@lynx-js/testing-environment'; + * import { JSDOM } from 'jsdom'; * * const lynxTestingEnv = new LynxTestingEnv(new JSDOM()); *
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/testing-library/testing-environment/package.json(1 hunks)packages/testing-library/testing-environment/src/index.ts(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/testing-library/testing-environment/package.json
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-11T05:57:18.212Z
Learnt from: upupming
PR: lynx-family/lynx-stack#1305
File: packages/testing-library/testing-environment/src/index.ts:255-258
Timestamp: 2025-08-11T05:57:18.212Z
Learning: In the ReactLynx testing environment (`packages/testing-library/testing-environment/src/index.ts`), the dual assignment pattern `target.console.method = console.method = () => {}` is required for rstest compatibility. This is because rstest provides `console` in an IIFE (Immediately Invoked Function Expression), and both the target and global console need to have these methods defined for proper test execution.
Applied to files:
packages/testing-library/testing-environment/src/index.ts
📚 Learning: 2025-08-13T11:50:58.069Z
Learnt from: colinaaa
PR: lynx-family/lynx-stack#1500
File: .changeset/curvy-dragons-appear.md:1-5
Timestamp: 2025-08-13T11:50:58.069Z
Learning: In the lynx-family/lynx-stack repository, all packages under packages/react/ should use "lynx-js/react" in changesets since they are all published together under the same package name, regardless of their individual package.json names. This includes packages/react/testing-library and other sub-packages.
Applied to files:
packages/testing-library/testing-environment/src/index.ts
🔇 Additional comments (1)
packages/testing-library/testing-environment/src/index.ts (1)
457-465: Constructor precedence and runtime guard look correctExplicit instance is preferred over the global fallback, and a clear error is thrown when neither is provided. This addresses the earlier precedence concern and avoids a crash on access.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @lynx-js/react@0.112.3 ### Patch Changes - fix css transform error in testing library ([#1500](#1500)) - fix the type error of `wrapper` option in testing library's `render` and `renderHook` function ([#1502](#1502)) - Introduce recursive hydration for lists to prevent double remove/insert on moves. ([#1401](#1401)) - Handle `<frame/>` correctly. ([#1497](#1497)) ## @lynx-js/rspeedy@0.10.7 ### Patch Changes - `output.inlineScripts` defaults to `false` when chunkSplit strategy is not `'all-in-one'` ([#1504](#1504)) ## @lynx-js/react-rsbuild-plugin@0.10.12 ### Patch Changes - `output.inlineScripts` defaults to `false` when chunkSplit strategy is not `'all-in-one'` ([#1504](#1504)) - Updated dependencies \[[`51a0b19`](51a0b19), [`b391ef5`](b391ef5)]: - @lynx-js/template-webpack-plugin@0.8.4 - @lynx-js/css-extract-webpack-plugin@0.6.2 - @lynx-js/react-alias-rsbuild-plugin@0.10.12 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-refresh-webpack-plugin@0.3.4 - @lynx-js/react-webpack-plugin@0.6.19 ## @lynx-js/testing-environment@0.1.6 ### Patch Changes - Fix that `lynxTestingEnv.jsdom` cannot be initialized correctly when `global.jsdom` is not defined. ([#1422](#1422)) ## @lynx-js/web-constants@0.15.6 ### Patch Changes - fix: systeminfo in mts function ([#1537](#1537)) - feat: add MTS API: \_\_UpdateComponentInfo ([#1485](#1485)) - fix: `__ElementFromBinary` needs to correctly apply the dataset in elementTemplate to the Element ([#1487](#1487)) - fix: all attributes except `id` and `type` under ElementTemplateData are optional. ([#1483](#1483)) - feat: add MTS API \_\_GetAttributeByName ([#1486](#1486)) - Updated dependencies \[]: - @lynx-js/web-worker-rpc@0.15.6 ## @lynx-js/web-core@0.15.6 ### Patch Changes - fix: systeminfo in mts function ([#1537](#1537)) - refactor: use utf-8 string ([#1473](#1473)) - Fix mtsGlobalThis race condition in createRenderAllOnUI ([#1506](#1506)) - Updated dependencies \[[`405a917`](405a917), [`b8f89e2`](b8f89e2), [`f76aae9`](f76aae9), [`b8b060b`](b8b060b), [`d8381a5`](d8381a5), [`214898b`](214898b), [`ab8cee4`](ab8cee4)]: - @lynx-js/web-mainthread-apis@0.15.6 - @lynx-js/web-constants@0.15.6 - @lynx-js/web-worker-runtime@0.15.6 - @lynx-js/web-worker-rpc@0.15.6 ## @lynx-js/web-core-server@0.15.6 ### Patch Changes - refactor: use utf-8 string ([#1473](#1473)) ## @lynx-js/web-mainthread-apis@0.15.6 ### Patch Changes - fix: systeminfo in mts function ([#1537](#1537)) - refactor: use utf-8 string ([#1473](#1473)) - feat: add MTS API: \_\_UpdateComponentInfo ([#1485](#1485)) - fix: \_\_ElementFromBinary should mark all elements actively ([#1484](#1484)) - fix: `__ElementFromBinary` needs to correctly apply the dataset in elementTemplate to the Element ([#1487](#1487)) - fix: all attributes except `id` and `type` under ElementTemplateData are optional. ([#1483](#1483)) - feat: add MTS API \_\_GetAttributeByName ([#1486](#1486)) - Updated dependencies \[[`405a917`](405a917), [`b8f89e2`](b8f89e2), [`f76aae9`](f76aae9), [`d8381a5`](d8381a5), [`214898b`](214898b), [`ab8cee4`](ab8cee4)]: - @lynx-js/web-constants@0.15.6 - @lynx-js/web-style-transformer@0.15.6 ## @lynx-js/web-style-transformer@0.15.6 ### Patch Changes - refactor: use utf-8 string ([#1473](#1473)) ## @lynx-js/web-worker-runtime@0.15.6 ### Patch Changes - Updated dependencies \[[`405a917`](405a917), [`b8f89e2`](b8f89e2), [`f76aae9`](f76aae9), [`b8b060b`](b8b060b), [`d8381a5`](d8381a5), [`214898b`](214898b), [`ab8cee4`](ab8cee4)]: - @lynx-js/web-mainthread-apis@0.15.6 - @lynx-js/web-constants@0.15.6 - @lynx-js/web-worker-rpc@0.15.6 ## @lynx-js/css-extract-webpack-plugin@0.6.2 ### Patch Changes - Fix "emit different content to the same filename" error ([#1482](#1482)) ## @lynx-js/template-webpack-plugin@0.8.4 ### Patch Changes - Fix invalid `module.exports=;` syntax in app-service.js. ([#1501](#1501)) ## create-rspeedy@0.10.7 ## @lynx-js/react-alias-rsbuild-plugin@0.10.12 ## upgrade-rspeedy@0.10.7 ## @lynx-js/web-worker-rpc@0.15.6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary by CodeRabbit
Bug Fixes
Documentation
Refactor
Chores
Checklist