feat(react): createPortal via nodesRef patch ops#2543
Conversation
🦋 Changeset detectedLatest commit: e2c5cbd The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
67fc30b to
94f9f0b
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67fc30b68a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b424bce to
1565409
Compare
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
aa72c8d to
2e85cbe
Compare
React Example#7727 Bundle Size — 227.33KiB (+0.85%).e2c5cbd(current) vs 29ba9ba main#7715(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
React MTF Example#859 Bundle Size — 198.49KiB (+0.97%).e2c5cbd(current) vs 29ba9ba main#847(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
React External#842 Bundle Size — 685.19KiB (+0.7%).e2c5cbd(current) vs 29ba9ba main#830(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
Web Explorer#9300 Bundle Size — 900.03KiB (~+0.01%).e2c5cbd(current) vs 29ba9ba main#9288(baseline) Bundle metrics
Bundle size by type
Bundle analysis report Branch feat-react-portal-patch-channel Project dashboard Generated by RelativeCI Documentation Report issue |
d9ff5c1 to
6bbc286
Compare
Renders a vnode subtree into a different ReactLynx element identified by
a `NodesRef` (from `ref={setX}` or `lynx.createSelectorQuery()`), without
requiring any compile-time marker attribute. Implementation routes portal
ops through the existing SnapshotInstance/patch abstraction:
- New `nodesRefInsertBefore` / `nodesRefRemoveChild` patch ops; carried
via the regular `LifecycleConstant.patchUpdate` channel alongside BSI
CreateElement / InsertBefore / RemoveChild ops.
- `fakeRoot.insertBefore` wires `child.__parent = fakeRoot` so preact's
`removeNode` (which walks `child.parentNode.removeChild`) routes through
portal removeChild, otherwise unmount silently no-ops.
- Pre-hydrate Portal mounts queue into `pendingInsertBefore`;
`clearPendingPortalInsertBefore` (called from hydrate) replays the BSI
subtree's dropped CreateElement / SetAttributes / internal InsertBefore
ops via `reconstructInstanceTree`, then attaches the subtree to host
via `nodesRefInsertBefore`.
- `reconstructInstanceTree` extracted to its own module so portal's
pre-hydrate replay can share the helper without forming an import
cycle with `backgroundSnapshot.ts`.
Different design from #2501 (which uses a `portal-container` SWC
transform to lift the host subtree into a separate snapshot) — this one
stays inside the existing SnapshotInstance/patch model so hydrate diff
and future first-screen-direct-render paths can be reused without
protocol changes.
Tests cover pre-/post-hydrate mount, unmount via `componentWillUnmount`,
container swap, multi-child reorder + prepend, context propagation
across portal boundary, ctx-not-found soft-fail on apply, and host
selector miss; runtime test env gets `__GetPageElement` /
`__QuerySelector` mocks. testing-library suite includes a preact-parity
case ported from internal-preact's `feat/portal-slot` branch verifying
that portal content stays put while host's normal children toggle.
6bbc286 to
ffc0600
Compare
`nodesRefInsertBefore` / `nodesRefRemoveChild` previously soft-failed when the selector didn't resolve or the BSI subtree wasn't materialized. That's a caller bug (stale `NodesRef`), so use non-null assertions and throw instead of silently dropping the op.
- Throw meaningful errors (not raw `TypeError`s) when `nodesRefInsertBefore` / `nodesRefRemoveChild` apply hits a caller-bug state — message names the op, the childId, the selector, and hints at the most likely cause. - Move the portal-only apply handlers + selector lookup into a new `nodesRefApply.ts` module so `snapshotPatchApply.ts` stays focused on snapshot-tree ops. - Reject non-CSS-selector `NodesRef`s (`selectReactRef`, `selectUniqueID`) in `serializeNodesRef` instead of silently no-oping on the main thread. - Drain matching pending inserts from `pendingInsertBefore` when a portal child is unmounted before hydrate, so the queue replay during hydrate doesn't resurrect a child that was already torn down on background. - Switch the container-swap test to assert the portaled subtree actually moves from container A to container B. - Use `toThrowErrorMatchingInlineSnapshot` for the throw-path tests.
## Summary [#2538](#2538) dropped `//#build` from `build.dependsOn` to reduce cache fanout. `//#build` runs the root `tsc --build`, which produces the composite-project `lib/` outputs that several workspace packages declare as their public types: - `@lynx-js/rspeedy` → `"types": "./lib/index.d.ts"` - `@lynx-js/template-webpack-plugin`, `@lynx-js/web-rsbuild-server-middleware`, etc. `//#build` still runs as part of `pnpm turbo build` (no filter), but now in parallel with package builds. Two packages whose `tsgo` dts generation imports types from those `lib/` outputs race against `//#build`: - `@lynx-js/config-rsbuild-plugin` (imports `@lynx-js/rspeedy`, `@lynx-js/template-webpack-plugin` in `LynxConfigWebpackPlugin.ts` / `pluginLynxConfig.ts`) - `@lynx-js/reactlynx-testing-library` (imports `@lynx-js/rspeedy` in `rstest-config.ts`) When the race is lost, those builds fail with `TS2307: Cannot find module …`. This was hit on PR #2543 ([build / Build (Ubuntu)](https://github.com/lynx-family/lynx-stack/actions/runs/25103126514/job/73557596812) — same failure on Windows). ## Fix Add `//#build` to the `dependsOn` of just the two affected packages (not to the global `build.dependsOn`) so they wait for `tsc --build` without re-introducing the cache fanout #2538 was avoiding. ## Test plan - [x] ```bash find packages -name "*.tsbuildinfo" -delete find packages -type d -name lib -exec rm -rf {} + pnpm turbo build --force ``` succeeds end-to-end (49/49 tasks). On `main` (without this fix) the same command fails on `@lynx-js/config-rsbuild-plugin#build` and `@lynx-js/reactlynx-testing-library#build`. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Resolved TypeScript type resolution failures in clean CI/build environments. * **Chores** * Ensured root composite build runs before package builds to enforce correct build ordering and artifact availability. * Added build configuration for a new extractor package and updated package build dependencies. * **Documentation** * Clarified maintenance guidance for produced/generated build artifacts and cache outputs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Adds two cross-component tests in `react/testing-library` that drive the
portal end-to-end via `lynx.createSelectorQuery().select('#id')` instead
of a React ref — covers the CSS-selector `NodesRef` apply path that
real apps hit when the host and the consumer don't share a ref tree.
Also fixes the testing-library mock to match real Lynx behavior:
`select(selector)` now stores the CSS selector string in
`_nodeSelectToken.identifier` (instead of pre-resolving to the element's
unique id). `setNativeProps` and `fireEvent`'s `NodesRef` branch resolve
ID_SELECTOR tokens via `document.querySelector` and UNIQUE_ID tokens
via `__GetElementByUniqueId`, keeping existing tests passing.
`createPortal` is only invoked from the background thread — short-circuit to `null` on main thread so preact's `render`/`createElement` and the BSI linkage helpers don't get pulled into the main-thread chunk. Fixes a typo (`if (__MAIN_THREAD__) null;` was a discarded expression that left the function falling through), updates the public api-extractor baseline (`VNode<any>` → `VNode<any> | null`), and updates the runtime test to assert the early-return on main thread + materialization on background thread. Also adds `nodesRefInsertBefore` / `nodesRefRemoveChild` to the `formatPatch.test.ts` fixture so the params-metadata for those entries is regression-checked.
Drops the api-extractor `ae-missing-release-tag` warning by tagging `createPortal` `@public`, matching how it's already exposed via `react.docs.d.ts`. Refreshes the api-extractor baseline accordingly.
Merging this PR will degrade performance by 12.74%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ❌ | 004-various-update__main-thread-setAttribute__TimingFlag |
74.3 µs | 80.5 µs | -7.7% |
| ❌ | 008-many-use-state-destroyBackground |
8 ms | 9.2 ms | -12.74% |
| ⚡ | basic-performance-text-200 |
12.1 ms | 11.2 ms | +7.89% |
Comparing feat-react-portal-patch-channel (e2c5cbd) with main (caadd3b)2
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. ↩
-
No successful run was found on
main(3841ffe) during the generation of this report, so caadd3b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
Summary
Adds
createPortalto@lynx-js/react. Renders a vnode subtree into a different ReactLynx element identified by aNodesRef(fromref={setX}orlynx.createSelectorQuery()), with no compile-time marker required and arbitrary host structure permitted.Different design vs #2501
This is an alternative implementation to #2501; both target the same feature with substantially different architectures. Brief comparison:
portal-containerattr requiredLifecycleConstant.patchUpdatechannelreconstructInstanceTreeConcretely, this implementation routes everything through
SnapshotInstanceso no new lifecycle protocol is introduced; the trade-off is that pre-hydrate Portal mounts need an extra queue-and-replay step (pendingInsertBefore→clearPendingPortalInsertBefore) instead of being lifted by a transform.Limitation
Currently
NodesRefis a BTS type, we cannot get it on MTS. So IFR is not supported.Implementation notes
nodesRefInsertBefore(identifier, childId, beforeId?)/nodesRefRemoveChild(identifier, childId). Carried via the existingpatchUpdatechannel alongside BSICreateElement/InsertBefore/RemoveChildops; no new protocol.fakeRoot.insertBeforewireschild.__parent = fakeRootso preact'sremoveNode(which walkschild.parentNode.removeChild(child)) routes through portalremoveChild. Without this, unmount silently no-ops.CreateElementpush is dropped (global buffer isundefined), sofakeRoot.insertBeforequeues intopendingInsertBefore.clearPendingPortalInsertBefore(called fromhydrate()) replays the dropped subtree ops viareconstructInstanceTree([child]), then emitsnodesRefInsertBeforeto attach to host.reconstructInstanceTreeextracted to its own module (snapshot/reconstructInstanceTree.ts) so portal pre-hydrate replay can share the helper without forming an import cycle withbackgroundSnapshot.ts.Test coverage
Two test suites:
runtime/__test__/snapshot/lynx/portals.test.jsx— unit tests on the runtime path (10 cases):createPortalreturns a VNode whosecontainerInfopoints at the host_this._container !== containerpath)before?.__idtruthy branch + apply__InsertElementBefore)ContextProviderwrapper)serializeNodesRefnon-RefProxy pathnodesRefInsertBefore/nodesRefRemoveChildctx-not-found soft-failtesting-library/src/__tests__/portals.test.jsx— high-level scenarios mirroring PR #2501's portal.test.jsx (16 cases): basic render, re-render on portalled state change, context forwarding, event-bubbling semantics across portal boundary, third-party-slot pattern, mount/unmount toggle, cleanup on unmount, host swap.Runtime test env (
__test__/snapshot/utils/nativeMethod.ts) gets__GetPageElement+__QuerySelectormocks ([attr]/[attr=value]selectors).Coverage: 100% lines / 100% branches / 100% functions / 100% statements on the runtime suite.
Test plan
pnpm -F @lynx-js/react-runtime test— 570 passing, 100% coveragepnpm -F @lynx-js/reactlynx-testing-library test:base -- src/__tests__/portals.test.jsx— 16 passing