feat(scripts): add async readiness and consumer scopes#840
Open
harlan-zw wants to merge 9 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesAsync script lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Consumer
participant FrameworkComposable
participant useScriptScope
participant SharedScript
participant ScopeEffect
Consumer->>FrameworkComposable: register scoped callbacks or effects
FrameworkComposable->>useScriptScope: create consumer scope
useScriptScope->>SharedScript: share script and register handlers
SharedScript->>ScopeEffect: invoke onLoadedEffect with signal
ScopeEffect->>Consumer: return cleanup
Consumer->>useScriptScope: dispose scope
useScriptScope->>ScopeEffect: abort and run cleanup
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
6 tasks
Contributor
📦 Bundle Size
All bundles (14)
⚡ Performance (directional)✅ No significant change (within CI noise) All benchmarks (14)
Baseline: main @ cc5127a · 2026-07-15 · gzipped is the headline size metric · perf is directional (shared-runner, gated) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/unhead/src/scripts/useScript.ts (1)
83-93: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winAlways return a disposer for duplicate keyed callbacks.
The SSR branch now honors the public disposer contract, but the duplicate-key branch at Line 92 still returns
undefined. Calling that typed disposer will throw.Proposed fix
if (options?.key) { uniqueKey = `${key}:${options.key}` if (_uniqueCbs.has(uniqueKey)) { - return + return () => {} }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/unhead/src/scripts/useScript.ts` around lines 83 - 93, Update _registerCb so the duplicate keyed-callback path returns a no-op disposer function instead of undefined, matching the existing SSR branch and preserving the typed disposer contract.
🧹 Nitpick comments (1)
packages/svelte/src/composables.ts (1)
66-72: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrevent memory leaks by removing manually invoked disposers from the component's cleanup array.
Both the Svelte and Angular
useScriptbindings maintain a localsideEffectsarray to automatically clean up script event listeners when the component unmounts. However, when the user manually invokes the returned disposer (for example, inside a reactive watcher or effect cleanup), the listener is detached but its reference remains insideEffects. If listeners are repeatedly registered and disposed over a long-lived component's lifetime, this array will grow unbounded, causing a memory leak.Update the returned disposers to splice their associated cleanup function out of the
sideEffectsarray.
packages/svelte/src/composables.ts#L66-L72: wrap the returnedofffunction to splice it fromsideEffectswhen called.packages/angular/src/composables.ts#L75-L83: update the returned disposer inside_registerCbto spliceofffromsideEffectsif it exists.♻️ Proposed fixes
packages/svelte/src/composables.ts
const bind = (base: (cb: any) => () => void) => (cb: any) => { const off = base(cb) sideEffects.push(off) - return off + return () => { + const idx = sideEffects.indexOf(off) + if (idx !== -1) + sideEffects.splice(idx, 1) + off() + } }packages/angular/src/composables.ts
Note: Modify the returned disposer starting at line 90 in the_registerCbfunction context.return () => { if (disposed) return disposed = true const idx = mountCbs.indexOf(run) if (idx !== -1) mountCbs.splice(idx, 1) + if (off) { + const sideEffectIdx = sideEffects.indexOf(off) + if (sideEffectIdx !== -1) + sideEffects.splice(sideEffectIdx, 1) + } off?.() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/svelte/src/composables.ts` around lines 66 - 72, Update the disposer wrapping in packages/svelte/src/composables.ts at lines 66-72 so the returned function removes its associated off callback from sideEffects before invoking it. Apply the same cleanup in packages/angular/src/composables.ts at lines 75-83 within _registerCb, splicing off from sideEffects when present; both sites must prevent manually disposed listeners from remaining in the cleanup array.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/unhead/src/scripts/types.ts`:
- Around line 143-146: Handle nullish use() results as terminal failures instead
of leaving load promises pending. In packages/unhead/src/scripts/types.ts lines
65-67, define the resolved API type as NonNullable<Awaited<ReturnType<V>>>; in
packages/unhead/src/scripts/types.ts lines 143-146, remove null and undefined
from the use() contract or classify them as failure; and in
packages/unhead/src/scripts/useScript.ts lines 162-170, add a terminal branch
for nullish outcome.api while preserving existing success handling.
In `@packages/unhead/src/scripts/useScript.ts`:
- Around line 321-346: Update the trigger execution flow in useScript so the
catch path fully tears down lifecycle state when trigger(script.load) throws,
including aborting the trigger controller, removing lifecycle signal/hook state,
and cleaning up any injected entry started by use() or load(). Ensure this
cleanup occurs before rethrowing and before useScript() can leave partial
registration.
- Around line 359-364: Update the terminal error replay in the script loading
flow so late onError subscribers receive the captured loadError, matching the
existing early callback behavior. Locate the _registerCb replay path and pass
loadError when invoking registered error callbacks instead of calling them
without an argument.
---
Outside diff comments:
In `@packages/unhead/src/scripts/useScript.ts`:
- Around line 83-93: Update _registerCb so the duplicate keyed-callback path
returns a no-op disposer function instead of undefined, matching the existing
SSR branch and preserving the typed disposer contract.
---
Nitpick comments:
In `@packages/svelte/src/composables.ts`:
- Around line 66-72: Update the disposer wrapping in
packages/svelte/src/composables.ts at lines 66-72 so the returned function
removes its associated off callback from sideEffects before invoking it. Apply
the same cleanup in packages/angular/src/composables.ts at lines 75-83 within
_registerCb, splicing off from sideEffects when present; both sites must prevent
manually disposed listeners from remaining in the cleanup array.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 467321cd-07ca-49ba-b020-0bf53f615aaa
📒 Files selected for processing (10)
docs/head/7.api/composables/4.use-script.mdpackages/angular/src/composables.tspackages/svelte/src/composables.tspackages/unhead/src/scripts/types.tspackages/unhead/src/scripts/useScript.tspackages/unhead/test/unit/scripts/events.test.tspackages/unhead/test/unit/scripts/proxy.test.tspackages/unhead/test/unit/scripts/use.test.tspackages/vue/src/scripts/index.tspackages/vue/src/scripts/useScript.ts
Collaborator
Author
|
Addressed the two non-inline review findings in
Focused lint, 35 lifecycle/framework tests, and workspace typecheck pass locally. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/svelte/src/composables.ts`:
- Around line 66-67: Update the Svelte composables’ bind helper to accept the
registration options and forward them to the base callback registration
function, preserving keyed deduplication. Apply the same change to the Angular
composables’ baseOnLoaded and baseOnError registration calls so their options
are not dropped.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba563624-6bb0-4b36-83e9-9078b1650e2f
📒 Files selected for processing (5)
packages/angular/src/composables.tspackages/svelte/src/composables.tspackages/unhead/src/scripts/types.tspackages/unhead/src/scripts/useScript.tspackages/unhead/test/unit/scripts/events.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/unhead/src/scripts/types.ts
- packages/angular/src/composables.ts
- packages/unhead/test/unit/scripts/events.test.ts
* feat(scripts): add consumer lifecycle scopes * fix(react): isolate script callback registrations * fix(scripts): harden scoped lifecycle facades * fix(scripts): guard callback and trigger results
harlan-zw
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/head/7.api/composables/4.use-script.md`:
- Around line 186-190: Update the useScript example so onLoadedEffect() is
called on a ScriptScope created via the controller’s createScope() method,
rather than directly on the generic script controller. Keep the existing
callback behavior unchanged and ensure the example matches the documented return
types.
In `@packages/react/src/composables.ts`:
- Around line 217-221: Update the handler and disposer logic in the composable
around the callback record so callback execution no longer marks the record
disposed before cleanup runs. Track callback invocation separately from
disposal, ensure record.off and record removal still execute when the disposer
is called afterward, and preserve keyed callback re-registration within the same
mounted component; add coverage for invoke, dispose, then register the same key
again.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e109425b-6b09-4b46-a190-7d60c29e87bc
📒 Files selected for processing (17)
docs/head/7.api/composables/4.use-script.mdpackages/angular/src/composables.tspackages/react/src/composables.tspackages/react/test/useScript.test.tsxpackages/solid-js/src/composables.tspackages/svelte/src/composables.tspackages/unhead/src/composables.tspackages/unhead/src/index.tspackages/unhead/src/scripts/index.tspackages/unhead/src/scripts/scope.tspackages/unhead/src/scripts/types.tspackages/unhead/src/scripts/useScript.tspackages/unhead/src/scripts/waitFor.tspackages/unhead/test/unit/scripts/events.test.tspackages/unhead/test/unit/scripts/scope.test.tspackages/unhead/test/unit/scripts/use.test.tspackages/vue/src/scripts/useScript.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/unhead/test/unit/scripts/use.test.ts
- packages/unhead/test/unit/scripts/events.test.ts
- packages/unhead/src/scripts/useScript.ts
Collaborator
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
use()await SDK readiness with a lifecycleAbortSignaland awaitFor()helperload(), callback replay, and proxy calls pending until the SDK API is actually readyScriptScopeinstances throughcreateScope()anduseScriptScope()onLoadedEffect()for resources whose cleanup belongs to one consumerWhy
The script element and SDK API are shared, but a component's callbacks and resources are not. Integrations were rebuilding the same promise, abort, settled-state, callback-restoration, and cleanup machinery, then attaching consumer state to a cached
ScriptInstance.This separates those lifetimes.
use({ signal, waitFor })owns shared script-to-API readiness. A scope owns one consumer's registrations, async effects, abort signal, and disposal. Framework adapters can now bind that scope directly to their native lifecycle.Validation
pnpm test— 203 test files and 1,645 tests passed; 8 skippedSummary by CodeRabbit
New Features
useScriptScopefor independently managing shared script consumers and cleanup.waitFor()and cancellation support.onLoadedEffect().Documentation
Tests