Skip to content

refactor(react): Simplify onError and onSuccess handlers in lazy loading #1945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
manudeli wants to merge 3 commits into
base: feat/stable-lazy
Choose a base branch
from
+43 −26
Draft

refactor(react): Simplify onError and onSuccess handlers in lazy loading #1945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
92edcc3
refactor(react): Simplify onError and onSuccess handlers in lazy loading
manudeli Apr 3, 2026
ac28989
fix(react): Rename reloadOnErrorStorageKeyAccessKeySymbol to reloadOn…
manudeli Apr 3, 2026
73fcb86
refactor(react): Remove reloadOnErrorStorageKeySymbol from onError ex…
manudeli Apr 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions docs/suspensive.org/src/content/en/docs/react/lazy.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ const VersionSkewSafeComponent = lazy(
+ retryDelay: 1000,
+ }))
- const Component = lazy(() => import('./Component'), {
- onError: ({ error, load }) => {
- onError: ({ error }) => {
- const reloadKey = 'component_reload_count'
- const currentCount = parseInt(sessionStorage.getItem(reloadKey) || '0')
- const maxRetries = 3
Expand All @@ -141,7 +141,7 @@ const VersionSkewSafeComponent = lazy(
- }, 1000)
- }
- },
- onSuccess: ({ load }) => {
- onSuccess: () => {
- // Clear reload count on success
- sessionStorage.removeItem('component_reload_count')
- },
Expand Down
4 changes: 2 additions & 2 deletions docs/suspensive.org/src/content/ko/docs/react/lazy.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ const VersionSkewSafeComponent = lazy(
+ retryDelay: 1000,
+ }))
- const Component = lazy(() => import('./Component'), {
- onError: ({ error, load }) => {
- onError: ({ error }) => {
- const reloadKey = 'component_reload_count'
- const currentCount = parseInt(sessionStorage.getItem(reloadKey) || '0')
- const maxRetries = 3
Expand All @@ -141,7 +141,7 @@ const VersionSkewSafeComponent = lazy(
- }, 1000)
- }
- },
- onSuccess: ({ load }) => {
- onSuccess: () => {
- // 성공 시 재시도 횟수 초기화
- sessionStorage.removeItem('component_reload_count')
- },
Expand Down
16 changes: 8 additions & 8 deletions packages/react/src/lazy.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,7 +317,7 @@ describe('lazy', () => {
expect(screen.getByText('error')).toBeInTheDocument()

expect(onError).toHaveBeenCalledTimes(1)
expect(onError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(onError).toHaveBeenCalledWith(expect.objectContaining({ error: expect.any(Error) }))
})

it('should execute component onError first, then default onError', async () => {
Expand Down Expand Up @@ -615,7 +615,7 @@ describe('lazy', () => {
const mockImport = importCache.createImport({ failureCount: 10, failureDelay: 100, successDelay: 50 })
const Component = lazy(() => mockImport('/test-component'))

// Get the load function and set storage to the limit
// Set storage to the limit using the original load's toString (which is the storage key)
const loadFunction = Component.load
storage.setItem(loadFunction.toString(), '1')

Expand Down Expand Up @@ -717,7 +717,7 @@ describe('lazy', () => {

// Component's onError should also be called
expect(individualOnError).toHaveBeenCalledTimes(1)
expect(individualOnError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(individualOnError).toHaveBeenCalledWith(expect.objectContaining({ error: expect.any(Error) }))
await act(() => vi.advanceTimersByTimeAsync(1))
expect(mockReload).toHaveBeenCalledTimes(1)
})
Expand Down Expand Up @@ -749,7 +749,7 @@ describe('lazy', () => {

// Component's onError should also be called
expect(individualOnError).toHaveBeenCalledTimes(1)
expect(individualOnError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(individualOnError).toHaveBeenCalledWith(expect.objectContaining({ error: expect.any(Error) }))
await act(() => vi.advanceTimersByTimeAsync(1))
// reloadOnError should work
expect(mockReload).toHaveBeenCalledTimes(1)
Expand Down Expand Up @@ -788,9 +788,9 @@ describe('lazy', () => {

// Factory's onError should also be called
expect(defaultOnError).toHaveBeenCalledTimes(1)
expect(defaultOnError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(defaultOnError).toHaveBeenCalledWith(expect.objectContaining({ error: expect.any(Error) }))
expect(individualOnError).toHaveBeenCalledTimes(1)
expect(individualOnError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(individualOnError).toHaveBeenCalledWith(expect.objectContaining({ error: expect.any(Error) }))
expect(defaultOnSuccess).toHaveBeenCalledTimes(0)
expect(individualOnSuccess).toHaveBeenCalledTimes(0)
await act(() => vi.advanceTimersByTimeAsync(1))
Expand All @@ -815,8 +815,8 @@ describe('lazy', () => {

expect(defaultOnError).toHaveBeenCalledTimes(1)
expect(individualOnError).toHaveBeenCalledTimes(1)
expect(defaultOnSuccess).toHaveBeenCalledWith({ load: expect.any(Function) })
expect(individualOnSuccess).toHaveBeenCalledWith({ load: expect.any(Function) })
expect(defaultOnSuccess).toHaveBeenCalledTimes(1)
expect(individualOnSuccess).toHaveBeenCalledTimes(1)
expect(mockReload).toHaveBeenCalledTimes(1)
})
})
Expand Down
45 changes: 31 additions & 14 deletions packages/react/src/lazy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
'use client'
import { type ComponentType, type LazyExoticComponent, lazy as originalLazy } from 'react'

const reloadOnErrorStorageKeySymbol = Symbol('reloadOnErrorStorageKey')
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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
}
Comment on lines 6 to 9
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

/**
Expand Down Expand Up @@ -47,14 +49,29 @@ export const createLazy =
): LazyExoticComponent<T> & {
load: () => Promise<{ default: T }>
} => {
const storageKey = load.toString()

const composedOnSuccess = () => {
options?.onSuccess?.({ load })
defaultOptions.onSuccess?.({ load })
;(options?.onSuccess as undefined | ((arg: { [reloadOnErrorStorageKeySymbol]: string }) => void))?.({
[reloadOnErrorStorageKeySymbol]: storageKey,
})
;(defaultOptions.onSuccess as undefined | ((arg: { [reloadOnErrorStorageKeySymbol]: string }) => void))?.({
[reloadOnErrorStorageKeySymbol]: storageKey,
})
}

const composedOnError = (error: unknown) => {
options?.onError?.({ error, load })
defaultOptions.onError?.({ error, load })
;(options?.onError as undefined | ((arg: { error: unknown; [reloadOnErrorStorageKeySymbol]: string }) => void))?.(
{
error,
[reloadOnErrorStorageKeySymbol]: storageKey,
}
)
;(
defaultOptions.onError as
| undefined
| ((arg: { error: unknown; [reloadOnErrorStorageKeySymbol]: string }) => void)
)?.({ error, [reloadOnErrorStorageKeySymbol]: storageKey })
}

return Object.assign(
Expand Down Expand Up @@ -188,14 +205,14 @@ export const reloadOnError = ({

return {
...options,
onSuccess: ({ load }) => {
options.onSuccess?.({ load })
reloadStorage.removeItem(load.toString())
},
onError: ({ error, load }) => {
options.onError?.({ error, load })

const storageKey = load.toString()
onSuccess: ((arg: any) => {
;(options.onSuccess as any)?.(arg)
const storageKey = arg?.[reloadOnErrorStorageKeySymbol] as string
reloadStorage.removeItem(storageKey)
}) as LazyOptions['onSuccess'],
Comment on lines +208 to +212
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
onError: (errorOptions: { error: unknown }) => {
options.onError?.(errorOptions)
const storageKey = (errorOptions as any)[reloadOnErrorStorageKeySymbol] as string
Comment on lines +210 to +215
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
let currentRetryCount = 0

Comment on lines +213 to 217
Copy link

Copilot AI Apr 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
if (typeof retry === 'number') {
Expand Down
Loading