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 1 commit
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
35 changes: 26 additions & 9 deletions packages/react/src/lazy.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { act, render, screen } from '@testing-library/react'
import { type ComponentType, Suspense, useState } from 'react'
import { afterEach, describe, expect, it, vi } from 'vitest'
import { ErrorBoundary } from './ErrorBoundary'
import { createLazy, lazy, reloadOnError } from './lazy'
import { createLazy, lazy, reloadOnError, reloadOnErrorStorageKeyAccessKeySymbol } from './lazy'
import { sleep } from './test-utils'

type PathData = {
Expand Down Expand Up @@ -317,7 +317,10 @@ 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({
error: expect.any(Error),
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
})

it('should execute component onError first, then default onError', async () => {
Expand Down Expand Up @@ -615,7 +618,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 +720,10 @@ 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({
error: expect.any(Error),
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
await act(() => vi.advanceTimersByTimeAsync(1))
expect(mockReload).toHaveBeenCalledTimes(1)
})
Expand Down Expand Up @@ -749,7 +755,10 @@ 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({
error: expect.any(Error),
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
await act(() => vi.advanceTimersByTimeAsync(1))
// reloadOnError should work
expect(mockReload).toHaveBeenCalledTimes(1)
Expand Down Expand Up @@ -788,9 +797,15 @@ 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({
error: expect.any(Error),
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
expect(individualOnError).toHaveBeenCalledTimes(1)
expect(individualOnError).toHaveBeenCalledWith({ error: expect.any(Error), load: expect.any(Function) })
expect(individualOnError).toHaveBeenCalledWith({
error: expect.any(Error),
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
expect(defaultOnSuccess).toHaveBeenCalledTimes(0)
expect(individualOnSuccess).toHaveBeenCalledTimes(0)
await act(() => vi.advanceTimersByTimeAsync(1))
Expand All @@ -815,8 +830,10 @@ 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).toHaveBeenCalledWith({ [reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String) })
expect(individualOnSuccess).toHaveBeenCalledWith({
[reloadOnErrorStorageKeyAccessKeySymbol]: expect.any(String),
})
expect(mockReload).toHaveBeenCalledTimes(1)
})
})
Expand Down
47 changes: 33 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'

export const reloadOnErrorStorageKeyAccessKeySymbol = Symbol('reloadOnErrorStorageKeyAccessKey')
Comment thread
manudeli marked this conversation as resolved.
Outdated

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,31 @@ 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: { [reloadOnErrorStorageKeyAccessKeySymbol]: string }) => void))?.({
[reloadOnErrorStorageKeyAccessKeySymbol]: storageKey,
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.

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.

Copilot uses AI. Check for mistakes.
})
;(
defaultOptions.onSuccess as undefined | ((arg: { [reloadOnErrorStorageKeyAccessKeySymbol]: string }) => void)
)?.({ [reloadOnErrorStorageKeyAccessKeySymbol]: storageKey })
}

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

return Object.assign(
Expand Down Expand Up @@ -188,14 +207,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?.[reloadOnErrorStorageKeyAccessKeySymbol] 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)[reloadOnErrorStorageKeyAccessKeySymbol] as string
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