fix(template-webpack-plugin): make getLynxTemplatePluginHooks a cross-module singleton#2624
Conversation
…-module singleton
When two physical copies of `@lynx-js/template-webpack-plugin` end up in
node_modules at distinct paths (e.g. npm hoist conflict that keeps an older
version pinned at the top by a peer dep range, while nested copies of the
current version live under different parents), Node's ESM loader treats
them as separate module instances. The module-scoped `LynxTemplatePluginHooksMap`
WeakMap is then duplicated per instance: taps registered through one copy
of `LynxEncodePlugin` are invisible to `hooks.encode.promise(...)` invoked
through another copy of `LynxTemplatePlugin`. The bail hook resolves with
`undefined`, surfacing as
Cannot destructure property 'buffer' of '(intermediate value)' as it is undefined.
Route storage through a `Symbol.for(...)` key stashed on the `compilation`
itself. `Symbol.for` returns the same symbol across module instances in a
realm, and the storage lives on the compilation (of which there is exactly
one), so any copy of the plugin resolves the same hooks for a given
compilation. pnpm installs were unaffected because their symlinked layout
already collapses multiple references to a single realpath, but the source
should not rely on the package manager's deduplication for correctness.
Adds a regression test (`test/get-hooks-singleton.test.ts`) that exercises
the cross-instance contract directly through the shared symbol key.
🦋 Changeset detectedLatest commit: 387bc34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
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 |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthrough
ChangesCross-Module Singleton Hook Storage
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Merging this PR will improve performance by 5.68%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | 008-many-use-state-destroyBackground |
9.5 ms | 8 ms | +19.24% |
| ❌ | transform 1000 view elements |
43.1 ms | 46 ms | -6.33% |
Tip
Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.
Comparing fix/template-hooks-singleton (387bc34) with main (bdbcf79)
Footnotes
-
26 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
React External#1286 Bundle Size — 693.04KiB (0%).387bc34(current) vs bdbcf79 main#1276(baseline) Bundle metrics
|
| Current #1286 |
Baseline #1276 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
17 |
17 |
|
5 |
5 |
|
8.59% |
8.59% |
|
0 |
0 |
|
0 |
0 |
Bundle analysis report Branch fix/template-hooks-singleton Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#8172 Bundle Size — 236.51KiB (0%).387bc34(current) vs bdbcf79 main#8161(baseline) Bundle metrics
|
| Current #8172 |
Baseline #8161 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
4 |
4 |
|
197 |
197 |
|
80 |
80 |
|
44.87% |
44.87% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #8172 |
Baseline #8161 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
90.75KiB |
90.75KiB |
Bundle analysis report Branch fix/template-hooks-singleton Project dashboard
Generated by RelativeCI Documentation Report issue
React Example with Element Template#439 Bundle Size — 197.79KiB (0%).387bc34(current) vs bdbcf79 main#428(baseline) Bundle metrics
Bundle size by type
|
| Current #439 |
Baseline #428 |
|
|---|---|---|
145.76KiB |
145.76KiB |
|
52.03KiB |
52.03KiB |
Bundle analysis report Branch fix/template-hooks-singleton Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9747 Bundle Size — 901.38KiB (0%).387bc34(current) vs bdbcf79 main#9736(baseline) Bundle metrics
|
| Current #9747 |
Baseline #9736 |
|
|---|---|---|
45.06KiB |
45.06KiB |
|
2.22KiB |
2.22KiB |
|
0% |
0% |
|
9 |
9 |
|
11 |
11 |
|
229 |
229 |
|
11 |
11 |
|
27.22% |
27.22% |
|
10 |
10 |
|
0 |
0 |
Bundle size by type no changes
| Current #9747 |
Baseline #9736 |
|
|---|---|---|
497.1KiB |
497.1KiB |
|
402.06KiB |
402.06KiB |
|
2.22KiB |
2.22KiB |
Bundle analysis report Branch fix/template-hooks-singleton Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#1305 Bundle Size — 207.46KiB (0%).387bc34(current) vs bdbcf79 main#1294(baseline) Bundle metrics
|
| Current #1305 |
Baseline #1294 |
|
|---|---|---|
0B |
0B |
|
0B |
0B |
|
0% |
0% |
|
0 |
0 |
|
3 |
3 |
|
192 |
192 |
|
77 |
77 |
|
44.38% |
44.38% |
|
2 |
2 |
|
0 |
0 |
Bundle size by type no changes
| Current #1305 |
Baseline #1294 |
|
|---|---|---|
111.23KiB |
111.23KiB |
|
96.23KiB |
96.23KiB |
Bundle analysis report Branch fix/template-hooks-singleton Project dashboard
Generated by RelativeCI Documentation Report issue
…a fixture The earlier singleton tests imported `LynxTemplatePlugin` from a single location and only reread the Symbol.for slot the same instance had just written, so they did not actually exercise two separate module instances. Add a fixture (`fixtures/template-plugin-second-instance.ts`) that re-implements `getLynxTemplatePluginHooks` end-to-end with its own `createHooks` and module-level state. Being a distinct file, Node's ESM loader treats it as a separate module instance — the same condition that arises when npm hoist conflicts place two physical copies of this package under node_modules. The new tests verify both write/read directions and one tap/await path, and were confirmed to fail against the pre-fix `WeakMap` implementation: the third test produces the exact `encode.promise()` resolves with `undefined` symptom from the original bug report.
…set (#2627) ## Summary Add a CI check that catches the case where a PR modifies a package's `dependencies` or `peerDependencies` but the changesets in that PR don't bump the affected package. Without this guard, the dep change lands on `main` and sits there until someone makes another change that triggers a release — silently shipping broken peer ranges and tripping the kind of npm hoist conflict that surfaced as #2624. ## Concrete motivation #2423 widened the `@lynx-js/react-webpack-plugin` peer range on `@lynx-js/react-refresh-webpack-plugin`, but the changeset only bumped `@lynx-js/react`, `@lynx-js/react-webpack-plugin`, and `@lynx-js/react-rsbuild-plugin`. The peer-range fix sat unreleased; `npm install` consumers continued to pull the old `@lynx-js/react-refresh-webpack-plugin@0.3.5` with the narrow peer range, force a parallel `@lynx-js/react-webpack-plugin@0.8.0` install, pin `@lynx-js/template-webpack-plugin@0.10.9` at the top of `node_modules` next to nested `0.11.0` copies, and ultimately produce the build error tracked in #2624. The missed-changeset follow-up is #2626. Running this new check against the `#2423` state (locally, with `--since=$(git rev-parse 1f4f117^)`) reproduces the catch: ``` The following packages changed `dependencies` or `peerDependencies` but no changeset bumps them: - @lynx-js/react-refresh-webpack-plugin [peerDependencies] packages/webpack/react-refresh-webpack-plugin/package.json Add a changeset (e.g. `pnpm changeset add`) bumping each affected package so the dependency change actually ships in the next release. ``` Exit code is non-zero — CI would have blocked the PR until either a changeset was added or the dep change was reverted. ## What the check does For each `package.json` modified relative to the PR's base ref: - Skip `private: true` packages (not published, so no release-time impact). - Skip files newly added in the PR (initial release is a separate workflow). - Diff `dependencies` and `peerDependencies` against the base. `devDependencies` is intentionally ignored — those don't affect downstream consumers. - If the diff is non-empty and the changeset status report does **not** list the package, flag it. - Print all flagged packages with the field that changed, then exit non-zero. `isShallowEqual` is tolerant of key reordering and ignores anything outside the two examined fields, so changes that don't actually move the resolution surface (formatting, version-field re-sorts via `pnpm meta-updater`) don't trip the check. ## Integration Inserted in the existing `code-style-check` job, immediately after `Changeset Heading Check`. The script's own unit-test file is also exercised in the same step via `node --test`, so the helper logic gets regression coverage without spinning up a separate runner. ## Test plan - [x] `node --test .github/scripts/check-dep-changes-have-changeset.test.cjs` — 10 cases, all pass (key reorder tolerance, dev-deps-only skip, private-pkg skip, newly-added-pkg skip, multi-package flagging, etc.) - [x] Running the script against the `#2423` state reproduces the catch and exits non-zero - [x] Running against this branch itself returns "all good" (no dep changes here) - [ ] CI pass on this PR <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Enhanced internal validation processes for dependency changes during the development workflow. --- **Note:** This release includes internal infrastructure improvements with no direct impact to end-user functionality. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/lynx-family/lynx-stack/pull/2627) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
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/autolink-codegen@0.1.0 ### Minor Changes - Add the Native Autolink codegen package. ([#2601](#2601)) ## create-lynx-extension@0.1.0 ### Minor Changes - Add the Native Autolink create-extension package. ([#2587](#2587)) ### Patch Changes - Use published package versions for scaffolded autolink codegen dependencies instead of workspace placeholders. ([#2628](#2628)) - Fix npm bin symlink entrypoint detection for the create extension CLI. ([#2623](#2623)) ## @lynx-js/react@0.121.0 ### Minor Changes - Support `React.createElement(type, props, children)` API. ([#2360](#2360)) ```jsx React.createElement("view", { style }, <text>hello</text>); // equivalent to <view style={style}> <text>hello</text> </view>; React.createElement(MyComponent, { style }, <view />); // equivalent to <MyComponent style={style}> <view /> </MyComponent>; ``` ### Patch Changes - Clear transient snapshot child props when removed snapshot subtrees are detached, preventing compiled `$*` child references from retaining deleted list holder or list item subtrees after removal. ([#2590](#2590)) - Add `createPortal` for rendering a subtree into a different ReactLynx element identified by a `NodesRef`. ([#2543](#2543)) ```tsx function App() { const [host, setHost] = useState(null); return ( <view> <view ref={setHost} /> {host && createPortal(<text>hi</text>, host)} </view> ); } ``` - Default `fireEvent` to `bubbles: true` for the TouchEvent family in testing-library to match Lynx runtime semantics, and stop reassigning the read-only `Event.prototype` accessors which threw `TypeError` in strict mode. ([#2532](#2532)) - Set `bundle-url` on lazy bundle border elements. ([#2537](#2537)) - Stop warning when `runWorklet` receives an invalid or missing main-thread function object. Invalid worklet contexts are still ignored, but nullish handler values no longer produce noisy `MainThreadFunction: Invalid function object` console output. ([#2586](#2586)) - Retain main-thread worklet context references before offscreen snapshot elements are materialized, so event, ref, gesture, and spread callbacks stay alive until the DOM update path can attach them. ([#2592](#2592)) - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) - Avoid retaining transformed nested worklet contexts after worklet transformation. ([#2591](#2591)) Nested worklets transformed by the worklet runtime now keep their context recovery metadata through a weak reference, preventing cached transformed worklet functions from keeping list-item worklet contexts alive. ## @lynx-js/docs-mcp-server@0.2.3 ### Patch Changes - fix(docs-mcp): recursively crawl and register nested llms.txt resources ([#2317](#2317)) ## @lynx-js/rspeedy@0.14.4 ### Patch Changes - feat(qrcode): support get entry from api exposed from rspeedy.env.entries ([#2551](#2551)) - Updated dependencies \[[`ad1f90f`](ad1f90f)]: - @lynx-js/chunk-loading-webpack-plugin@0.3.4 - @lynx-js/web-rsbuild-server-middleware@0.20.4 - @lynx-js/cache-events-webpack-plugin@0.0.3 ## @lynx-js/lynx-bundle-rslib-config@0.3.3 ### Patch Changes - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) ## @lynx-js/qrcode-rsbuild-plugin@0.4.7 ### Patch Changes - feat(qrcode): support get entry from api exposed from rspeedy.env.entries ([#2551](#2551)) ## @lynx-js/react-rsbuild-plugin@0.16.2 ### Patch Changes - Updated dependencies \[[`3e627b3`](3e627b3), [`7b8d63c`](7b8d63c), [`13a0776`](13a0776), [`a973c54`](a973c54), [`353b1b7`](353b1b7)]: - @lynx-js/template-webpack-plugin@0.11.1 - @lynx-js/react-refresh-webpack-plugin@0.3.6 - @lynx-js/react-alias-rsbuild-plugin@0.16.2 - @lynx-js/use-sync-external-store@1.5.0 - @lynx-js/react-webpack-plugin@0.9.2 - @lynx-js/css-extract-webpack-plugin@0.7.1 ## @lynx-js/web-core@0.20.4 ### Patch Changes - Always clone touch event lists when creating cross-thread events so synthetic touch events only carry structured-clone-safe primitive fields. ([#2636](#2636)) - Conditionally pass Card and Component params based on cardType in background thread. ([#2610](#2610)) - Add bidirectional decode worker heartbreak liveness messages. ([#2599](#2599)) - Add web support for the `<frame>` element by mapping it to `<lynx-view>`. ([#2604](#2604)) - Stop redeclaring `fetch` as a chunk-scope binding. Reusing the host ([#2562](#2562)) `window.fetch` from BTS chunks (instead of capturing the no-op stub the chunk wrapper used to install) lets the renderer issue real network requests. - Updated dependencies \[[`c1db603`](c1db603)]: - @lynx-js/web-elements@0.12.2 - @lynx-js/web-worker-rpc@0.20.4 ## @lynx-js/web-elements@0.12.2 ### Patch Changes - fix: xmarkdown create img incorrectly ([#2540](#2540)) ## @lynx-js/chunk-loading-webpack-plugin@0.3.4 ### Patch Changes - Override `__webpack_require__.e` so a single sync-then chunk load (the ([#2597](#2597)) typical lazy bundle case) bypasses `Promise.all`. It will make first screen in main thread can load lazy bundle synchronously when using dynamic import. ## @lynx-js/react-refresh-webpack-plugin@0.3.6 ### Patch Changes - Widen `@lynx-js/react-webpack-plugin` peer range to include `^0.9.0`. ([#2626](#2626)) ## @lynx-js/template-webpack-plugin@0.11.1 ### Patch Changes - feat(web): enable web binary template by default ([#2545](#2545)) The default encoding format for the web platform template has been changed from JSON to Binary. **Benefits for developers:** - **Smaller output size:** Binary templates are more compact than JSON strings, reducing the final bundle size. - **Faster load performance:** Binary templates parse faster than JSON in the runtime, improving the time-to-interactive for web applications. **How to turn off this feature:** If you encounter any issues with the new binary template format, you can revert to the previous JSON format by setting the environment variable `EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE` to `'false'` or `'0'` before running your build commands. For example: `EXPERIMENTAL_USE_WEB_BINARY_TEMPLATE=false rspeedy build` **Upgrade to `@lynx-js/web-core@0.20.2` could support the new output format** See [`@lynx-js/web-core` Changelog](https://lynx-stack.dev/changelog/lynx-js--web-core) - Run TASM template encoding in a shared `tinypool` worker pool so multi-entry builds encode in parallel and watch-mode rebuilds reuse warm workers. ([#2634](#2634)) - Make `LynxTemplatePlugin.getLynxTemplatePluginHooks` a cross-module singleton so duplicate copies of this package (e.g. from npm hoist conflicts) share the same hooks per compilation. ([#2624](#2624)) - Update the @lynx-js/tasm dependency to 0.0.39 and align React template attribute descriptors with it. ([#2643](#2643)) - Updated dependencies \[[`ee79eff`](ee79eff), [`ded4de9`](ded4de9), [`cf01e94`](cf01e94), [`b989c1c`](b989c1c), [`8417e68`](8417e68)]: - @lynx-js/web-core@0.20.4 ## @lynx-js/react-umd@0.121.0 ## create-rspeedy@0.14.4 ## @lynx-js/react-alias-rsbuild-plugin@0.16.2 ## upgrade-rspeedy@0.14.4 ## @lynx-js/web-rsbuild-server-middleware@0.20.4 ## @lynx-js/web-worker-rpc@0.20.4 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Summary
When two physical copies of
@lynx-js/template-webpack-pluginland at distinct paths undernode_modules(e.g. npm hoist conflict that keeps an older version pinned at top ofnode_modulesvia a peer-dep range, while nested copies of the current version live under different parents), Node's ESM loader treats them as separate module instances. The module-scopedLynxTemplatePluginHooksMapWeakMap then doubles up: taps registered through one copy ofLynxEncodePluginare invisible tohooks.encode.promise(...)invoked through another copy ofLynxTemplatePlugin. The bail hook resolves withundefined, surfacing as:pnpm-managed installs are unaffected because pnpm's symlinked layout collapses multiple references to a single realpath. But the source code shouldn't rely on the package manager's deduplication for correctness —
npm installandyarn installflat layouts are common, and any third-party wrapper around@lynx-js/template-webpack-pluginthat adds itself as an extra nested consumer can trigger the dual-instance condition even on the OSS chain.Fix
Route hooks storage through a
Symbol.for(...)key stashed on thecompilationitself:Symbol.for(key)returns the same symbol across all module instances in a realm — any copy of the plugin loaded by Node resolves the same key.compilation(of which there is exactly one), so the keyed slot is naturally per-compilation.Test plan
pnpm --filter @lynx-js/template-webpack-plugin test— all 348 tests pass locally, including 4 new cases intest/get-hooks-singleton.test.ts:Symbol.forkeytapregistered through one instance is reached whenencode.promise(...)is awaited through another@lynx-js/template-webpack-plugin@xlands at a distinct physical path; OSScreate-rspeedy@latestscaffolds did not surface the bug under any of pnpm/npm/yarn/bun. The fix is intended to be defensive — once merged, any future wrapper layer that creates a dual-instance condition will no longer be load-bearing for correctness.Notes
Draft to gather CI feedback. Happy to add an integration-style test that boots two physically-distinct copies via a synthetic monorepo if the maintainers want stronger coverage than the symbol-key contract test.
Summary by CodeRabbit
Release Notes