refactor(react): Simplify onError and onSuccess handlers in lazy loading#1945
refactor(react): Simplify onError and onSuccess handlers in lazy loading#1945manudeli wants to merge 3 commits intofeat/stable-lazyfrom
Conversation
People can be co-author:
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
There was a problem hiding this comment.
Pull request overview
This PR refactors the React lazy utilities to simplify the exposed onSuccess/onError callback shapes while still enabling reloadOnError to identify the correct retry storage key.
Changes:
- Simplified
LazyOptionscallback signatures and introduced a symbol-keyed metadata channel for passing the retry storage key internally. - Updated
reloadOnErrorto use the new symbol-keyed storage key rather than receivingloaddirectly. - Adjusted unit tests and documentation examples to reflect the new callback shapes.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/react/src/lazy.ts | Refactors callback typing/callback composition and changes how reloadOnError derives its storage key. |
| packages/react/src/lazy.spec.tsx | Updates expectations for onError/onSuccess calls to include the symbol-keyed storage key metadata. |
| docs/suspensive.org/src/content/ko/docs/react/lazy.mdx | Updates example callback signatures to remove load from handler params. |
| docs/suspensive.org/src/content/en/docs/react/lazy.mdx | Updates example callback signatures to remove load from handler params. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface LazyOptions { | ||
| onSuccess?: ({ load }: { load: () => Promise<{ default: ComponentType<any> }> }) => void | ||
| onError?: ({ error, load }: { error: unknown; load: () => Promise<{ default: ComponentType<any> }> }) => void | ||
| onSuccess?: () => void | ||
| onError?: (options: { error: unknown }) => void | ||
| } |
There was a problem hiding this comment.
LazyOptions['onSuccess'] is typed as () => void, but createLazy always calls it with an argument (currently an object keyed by reloadOnErrorStorageKeyAccessKeySymbol), forcing multiple unsafe casts. This also makes reloadOnError return a callback that expects an argument but is typed as no-arg, so calling it per the type would yield an undefined storage key. Consider changing onSuccess/onError types to accept an optional metadata argument (including the symbol-keyed storageKey) so createLazy and reloadOnError can be implemented without any/casts and the contract matches runtime behavior.
| const storageKey = load.toString() | ||
|
|
||
| const composedOnSuccess = () => { | ||
| options?.onSuccess?.({ load }) | ||
| defaultOptions.onSuccess?.({ load }) | ||
| ;(options?.onSuccess as undefined | ((arg: { [reloadOnErrorStorageKeyAccessKeySymbol]: string }) => void))?.({ | ||
| [reloadOnErrorStorageKeyAccessKeySymbol]: storageKey, |
There was a problem hiding this comment.
const storageKey = load.toString() is evaluated eagerly for every lazy component creation, even when no callbacks are provided. Since Function#toString() can be relatively expensive and can generate large strings, consider computing the storage key lazily (only inside composedOnSuccess/composedOnError when at least one callback is defined) to avoid unnecessary work.
| onSuccess: ((arg: any) => { | ||
| ;(options.onSuccess as any)?.(arg) | ||
| const storageKey = arg?.[reloadOnErrorStorageKeyAccessKeySymbol] as string | ||
| reloadStorage.removeItem(storageKey) | ||
| }) as LazyOptions['onSuccess'], |
There was a problem hiding this comment.
reloadOnError's onSuccess reads arg?.[reloadOnErrorStorageKeyAccessKeySymbol] and passes it to removeItem without validating it. Because the returned callback is typed as () => void, a consumer could legally invoke it with no args and cause removeItem to receive undefined (or the string 'undefined' for DOM storage). Add a runtime guard (e.g., no-op or throw with a clear error) and/or adjust the callback type so the storage key is required when invoked internally.
|
Size Change: +112 B (+0.19%) Total Size: 60 kB
ℹ️ View Unchanged
|
…ErrorStorageKeySymbol
…pectations in tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
packages/react/src/lazy.spec.tsx:6
- Tests now import reloadOnErrorStorageKeySymbol and assert exact argument objects containing that symbol, which couples tests to an internal implementation detail and effectively requires the symbol to be part of the public API. Prefer asserting with partial matchers (e.g. expect.objectContaining({ error: expect.any(Error) })) and/or validating retry behavior via storage calls, so the symbol can remain private and refactors don't break tests unnecessarily.
import { createLazy, lazy, reloadOnError } from './lazy'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'use client' | ||
| import { type ComponentType, type LazyExoticComponent, lazy as originalLazy } from 'react' | ||
|
|
||
| const reloadOnErrorStorageKeySymbol = Symbol('reloadOnErrorStorageKey') |
There was a problem hiding this comment.
Exporting reloadOnErrorStorageKeySymbol appears to be only for plumbing/testing and leaks an internal implementation detail into the public API surface. Prefer keeping this symbol internal (non-exported) and adjust tests to assert via behavior (storage interactions) or use partial matchers (e.g., objectContaining) rather than depending on a public symbol.
| const storageKey = arg?.[reloadOnErrorStorageKeySymbol] as string | ||
| reloadStorage.removeItem(storageKey) | ||
| }) as LazyOptions['onSuccess'], | ||
| onError: (errorOptions: { error: unknown }) => { | ||
| options.onError?.(errorOptions) | ||
| const storageKey = (errorOptions as any)[reloadOnErrorStorageKeySymbol] as string |
There was a problem hiding this comment.
reloadOnError's onSuccess reads the storage key from arg?.[reloadOnErrorStorageKeySymbol] and then calls removeItem(storageKey). If the callback is ever invoked without that symbol property, this will pass undefined (coerced to 'undefined') and remove the wrong entry. Add a guard (return early / throw) when storageKey is missing, or compute a safe fallback key.
| const storageKey = arg?.[reloadOnErrorStorageKeySymbol] as string | |
| reloadStorage.removeItem(storageKey) | |
| }) as LazyOptions['onSuccess'], | |
| onError: (errorOptions: { error: unknown }) => { | |
| options.onError?.(errorOptions) | |
| const storageKey = (errorOptions as any)[reloadOnErrorStorageKeySymbol] as string | |
| const storageKey = arg?.[reloadOnErrorStorageKeySymbol] | |
| if (typeof storageKey !== 'string') return | |
| reloadStorage.removeItem(storageKey) | |
| }) as LazyOptions['onSuccess'], | |
| onError: (errorOptions: { error: unknown }) => { | |
| options.onError?.(errorOptions) | |
| const storageKey = (errorOptions as any)[reloadOnErrorStorageKeySymbol] | |
| if (typeof storageKey !== 'string') return |
| onError: (errorOptions: { error: unknown }) => { | ||
| options.onError?.(errorOptions) | ||
| const storageKey = (errorOptions as any)[reloadOnErrorStorageKeySymbol] as string | ||
| let currentRetryCount = 0 | ||
|
|
There was a problem hiding this comment.
reloadOnError's onError assumes errorOptions carries reloadOnErrorStorageKeySymbol; if it's missing, storageKey becomes undefined and getItem/setItem/removeItem will operate on the wrong key. Guard against a missing/invalid storageKey before using storage and consider making the key part of the typed onError context so this cannot happen silently.
No description provided.