feat(image): add loading src to display loading state #4783
Conversation
🦋 Changeset detectedLatest commit: b0cf753 The changes in this PR will be included in the next version bump. This PR includes changesets to release 14 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 |
|
Someone is attempting to deploy a commit to the HeroUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Rate limit exceeded@Arian94 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 1 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageComponent
participant useImageHook
User->>ImageComponent: Render with src, loadingSrc, fallbackSrc
ImageComponent->>useImageHook: Initialize with props
useImageHook->>ImageComponent: Returns state (loading, error, loaded)
alt While loading
ImageComponent->>User: Display loadingSrc image
else If error
ImageComponent->>User: Display fallbackSrc image
else When loaded
ImageComponent->>User: Display main src image
end
Assessment against linked issues
Suggested reviewers
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
apps/docs/content/components/image/customLoading.raw.jsx (1)
8-9: Consider using local assets for demo reliability.The demo uses external services for both the placeholder and the delayed image:
via.placeholder.comfor loading staterequestly.iofor simulating delayThis could make the demo unreliable if these services are down.
Consider using local assets hosted within the project for both the placeholder and the main image. For delay simulation, consider using a local development proxy or built-in delay mechanism.
packages/components/image/stories/image.stories.tsx (2)
125-125: Consider more reliable delay simulation method.Multiple stories use
requestly.iofor delay simulation which could make the stories unreliable in offline environments or if the service is down.Consider using React Suspense or a local delay mechanism for more reliable story demonstrations.
Also applies to: 146-147
165-177: Add story description for CustomLoadingAndFallback.The new story demonstrates an important use case but lacks a description of what it's showcasing.
Add a JSDoc comment explaining the purpose of this story and when to use both loadingSrc and fallbackSrc together.
packages/components/image/src/use-image.ts (1)
27-30: Enhance loadingSrc prop documentation.The current documentation is minimal. Consider adding:
- Usage example
- Default value
- Note about interaction with disableSkeleton
/** * A loading image. + * @example + * ```jsx + * <Image loadingSrc="/loading.gif" src="/main.jpg" /> + * ``` + * @default undefined + * @note When provided, disables the default skeleton loader */apps/docs/content/docs/components/image.mdx (1)
165-169: API Enhancement:loadingSrcProp Entry
The addition of theloadingSrcprop within the API table is well-documented. Its type, description, and default value are clearly stated, providing comprehensive guidance for developers on how to use this new feature. Consider verifying whether the default value ("-") accurately represents the intended default state (e.g., an empty string might be more conventional) if applicable.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
apps/docs/content/components/image/customLoading.raw.jsx(1 hunks)apps/docs/content/components/image/customLoading.ts(1 hunks)apps/docs/content/docs/components/image.mdx(2 hunks)packages/components/image/src/image.tsx(2 hunks)packages/components/image/src/use-image.ts(7 hunks)packages/components/image/stories/image.stories.tsx(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/docs/content/components/image/customLoading.ts
🔇 Additional comments (4)
packages/components/image/src/image.tsx (1)
16-16: LGTM! Clean integration of loadingSrc.The loadingSrc prop is well-integrated into the existing component structure, maintaining consistency with the current implementation pattern.
Also applies to: 49-49
packages/components/image/src/use-image.ts (1)
140-142: LGTM! Clear separation of loading states.The loading state logic is well-organized with clear separation between:
- Loading state with custom image
- Error state with fallback
- Default skeleton loader
apps/docs/content/docs/components/image.mdx (2)
66-73: Documentation Clarity: "Custom Loading Image" Section
The new section clearly explains the purpose of theloadingSrcprop and how it allows a custom loading image to be displayed while the main image is still loading. The use of a dedicated code demo and a note about the simulated network delay is consistent with other sections in the document.
172-176: Improved Clarity:fallbackSrcProp Description Update
The updated description for thefallbackSrcprop now specifies that it is used when the main image fails to load or encounters an error, which is clearer than the previous explanation. This update helps avoid ambiguity about the prop’s behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/docs/content/docs/components/image.mdx (1)
57-60: Add "the" before "Image component" for better readability.The sentence should read "The Image component has a built-in
skeletonanimation..."-Image component has a built-in `skeleton` animation to indicate the image is loading in case the `loadingSrc` is not defined. +The Image component has a built-in `skeleton` animation to indicate the image is loading in case the `loadingSrc` is not defined.🧰 Tools
🪛 LanguageTool
[uncategorized] ~58-~58: You might be missing the article “the” here.
Context: ...d} /> ### Animated (Skeleton) Loading Image component has a built-inskeletonani...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/docs/content/components/image/customLoading.raw.jsx(1 hunks)apps/docs/content/components/image/fallback.raw.jsx(1 hunks)apps/docs/content/components/image/index.ts(2 hunks)apps/docs/content/docs/components/image.mdx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/docs/content/components/image/customLoading.raw.jsx
🧰 Additional context used
🪛 LanguageTool
apps/docs/content/docs/components/image.mdx
[uncategorized] ~58-~58: You might be missing the article “the” here.
Context: ...d} /> ### Animated (Skeleton) Loading Image component has a built-in skeleton ani...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
🔇 Additional comments (5)
apps/docs/content/components/image/fallback.raw.jsx (1)
9-9: LGTM! Good example for demonstrating fallback behavior.The intentionally invalid image source effectively demonstrates the fallback functionality.
apps/docs/content/components/image/index.ts (1)
5-5: LGTM! Clean integration of the new custom loading example.The changes follow the existing pattern for importing and exporting examples.
Also applies to: 14-14
apps/docs/content/docs/components/image.mdx (3)
65-71: LGTM! Clear documentation for the new custom loading feature.The new section effectively explains the
loadingSrcprop with a practical example and network simulation note.
83-84: LGTM! Helpful note about combining loading and fallback states.Good addition clarifying that both states can be handled simultaneously.
164-169: LGTM! Well-documented API properties.The new
loadingSrcprop and updatedfallbackSrcdescription are clear and accurate.Also applies to: 173-173
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/components/image/__tests__/image.test.tsx (3)
30-39: Enhance loading state test coverage.The test could be improved in several ways:
- Add cleanup to prevent test pollution
- Add explicit null check instead of using non-null assertion
- Test the transition from loading to loaded state
Consider this improved version:
test("renders loading source while loading the image.", async () => { const wrapper = render(<Image loadingSrc={loadingSrc} src={src} />); const imageParent = wrapper.getByRole("img").parentElement; - expect(imageParent?.getAttribute("data-testid")).toEqual("heroUI/image_parent"); + expect(imageParent).not.toBeNull(); + expect(imageParent!.getAttribute("data-testid")).toEqual("heroUI/image_parent"); const computedStyle = window.getComputedStyle(imageParent!); expect(computedStyle.backgroundImage).toBe(`url(${loadingSrc})`); + + // Test transition to loaded state + fireEvent.load(wrapper.getByRole("img")); + await waitFor(() => { + const updatedStyle = window.getComputedStyle(imageParent!); + expect(updatedStyle.backgroundImage).toBe(`url(${src})`); + }); + + // Cleanup + wrapper.unmount(); });
41-55: Optimize error handling test.The test has some potential improvements:
- The 6-second timeout seems excessive for a unit test
- Missing cleanup
- Could use more specific error simulation
Consider this improved version:
test("renders fallback source if src is wrong or not found.", async () => { const onError = jest.fn(); const wrapper = render( <Image alt="test" fallbackSrc={fallbackSrc} src="wrong-src-address" onError={onError} />, ); const imageParent = wrapper.getByRole("img").parentElement; - fireEvent.error(wrapper.getByRole("img")); + // Simulate specific error type + fireEvent.error(wrapper.getByRole("img"), { + target: { error: new Error('Failed to load image') } + }); - await waitFor(() => expect(onError).toHaveBeenCalled(), {timeout: 5_000}); + await waitFor(() => expect(onError).toHaveBeenCalled(), {timeout: 1000}); expect(imageParent?.getAttribute("data-testid")).toEqual("heroUI/image_parent"); const computedStyle = window.getComputedStyle(imageParent!); expect(computedStyle.backgroundImage).toBe(`url(${fallbackSrc})`); -}, 6_000); + + // Cleanup + wrapper.unmount(); +}, 2000);
57-61: Add cleanup to prevent test pollution.The test is well-structured but should include cleanup to prevent potential test pollution.
test("renders image if there is no loading or fallback behavior defined", async () => { const wrapper = render(<Image src={src} />); expect(wrapper.getByRole("img")).toHaveAttribute("src", src); + wrapper.unmount(); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/components/image/__tests__/image.test.tsx(2 hunks)packages/components/image/src/image.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/components/image/src/image.tsx
🔇 Additional comments (1)
packages/components/image/__tests__/image.test.tsx (1)
8-8: Consider using an absolute URL for loadingSrc.Using a relative path
/images/local-image-small.jpgmight cause issues in different test environments. Consider using an absolute URL (like the placeholder URLs used forsrcandfallbackSrc) to ensure consistent test behavior across environments.
I couldn't. The second test needs the |
|
you may take a look at |
done. |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/image/__tests__/image.test.tsx(4 hunks)
🔇 Additional comments (3)
packages/components/image/__tests__/image.test.tsx (3)
50-71: LGTM! Well-structured test for loading state.The test thoroughly verifies both the loading state and the final loaded state, including:
- Loading image display
- onLoad callback
- Proper cleanup
110-115: LGTM! Clear test description update.The test description now accurately reflects the expanded scope including loading behavior.
33-33: LGTM! Consistent cleanup practice.Good addition of wrapper.unmount() calls across all test cases to prevent memory leaks and test interference.
Also applies to: 47-47, 70-70, 107-107, 114-114, 121-121, 129-129, 143-143, 164-164
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/components/image/__tests__/image.test.tsx (2)
30-38: Optimize cleanup code performance.The cleanup code is good but can be optimized by avoiding the delete operator.
afterAll(() => { - delete window.Image.prototype._onload; + window.Image.prototype._onload = undefined; Object.defineProperty(window.Image.prototype, "onload", { value: null, writable: true, configurable: true, }); });🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
84-130: Enhance error handling test.The test case is good but can be improved in two areas:
- Performance optimization for cleanup
- More robust error simulation
const cleanup = () => { - delete window.Image.prototype._onerror; + window.Image.prototype._onerror = undefined; Object.defineProperty(window.Image.prototype, "onerror", { value: null, writable: true, configurable: true, }); }; act(() => { - imageOnerror(); + imageOnerror(new Event('error')); });🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/components/image/__tests__/image.test.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/image/__tests__/image.test.tsx
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 103-103: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (4)
packages/components/image/__tests__/image.test.tsx (4)
7-8: LGTM! Constants are well-defined and descriptive.The constants are appropriately named and their values are clear and meaningful.
13-28: LGTM! Image onload tracking setup is well-implemented.The setup code properly tracks the image onload event by modifying the Image prototype, which is essential for testing loading states.
61-82: LGTM! Loading state test is comprehensive.The test case thoroughly verifies:
- Initial loading image display
- Proper loading state transition
- onLoad callback invocation
- Cleanup after test completion
132-137: LGTM! Test cases have proper cleanup.All test cases now include proper cleanup by unmounting components after test completion.
Also applies to: 139-187
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
@heroui/accordion
@heroui/alert
@heroui/autocomplete
@heroui/avatar
@heroui/badge
@heroui/breadcrumbs
@heroui/button
@heroui/calendar
@heroui/card
@heroui/checkbox
@heroui/chip
@heroui/code
@heroui/date-input
@heroui/date-picker
@heroui/divider
@heroui/drawer
@heroui/dropdown
@heroui/form
@heroui/image
@heroui/input
@heroui/input-otp
@heroui/kbd
@heroui/link
@heroui/listbox
@heroui/menu
@heroui/modal
@heroui/navbar
@heroui/number-input
@heroui/pagination
@heroui/popover
@heroui/progress
@heroui/radio
@heroui/ripple
@heroui/scroll-shadow
@heroui/select
@heroui/skeleton
@heroui/slider
@heroui/snippet
@heroui/spacer
@heroui/spinner
@heroui/switch
@heroui/table
@heroui/tabs
@heroui/toast
@heroui/tooltip
@heroui/user
@heroui/react
@heroui/system
@heroui/system-rsc
@heroui/theme
@heroui/use-aria-accordion
@heroui/use-aria-accordion-item
@heroui/use-aria-button
@heroui/use-aria-link
@heroui/use-aria-modal-overlay
@heroui/use-aria-multiselect
@heroui/use-callback-ref
@heroui/use-clipboard
@heroui/use-data-scroll-overflow
@heroui/use-disclosure
@heroui/use-draggable
@heroui/use-image
@heroui/use-infinite-scroll
@heroui/use-intersection-observer
@heroui/use-is-mobile
@heroui/use-is-mounted
@heroui/use-measure
@heroui/use-pagination
@heroui/use-real-shape
@heroui/use-ref-state
@heroui/use-resize
@heroui/use-safe-layout-effect
@heroui/use-scroll-position
@heroui/use-ssr
@heroui/use-theme
@heroui/use-update-effect
@heroui/aria-utils
@heroui/dom-animation
@heroui/framer-utils
@heroui/react-rsc-utils
@heroui/react-utils
@heroui/shared-icons
@heroui/shared-utils
@heroui/stories-utils
@heroui/test-utils
commit: |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
apps/docs/content/docs/components/image.mdx (1)
101-102: Fix grammatical error in slots description.There's a duplicate "the" in the fallbackImgWrapper description.
Apply this diff to fix the grammar:
-- **fallbackImgWrapper**: Applies styles to the the "wrapper" only in the failed state. +- **fallbackImgWrapper**: Applies styles to the "wrapper" only in the failed state.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
apps/docs/content/components/image/customLoading.raw.jsx(1 hunks)apps/docs/content/components/image/fallback.raw.jsx(1 hunks)apps/docs/content/docs/components/image.mdx(4 hunks)packages/components/image/__tests__/image.test.tsx(2 hunks)packages/components/image/src/image.tsx(2 hunks)packages/components/image/src/use-image.ts(7 hunks)packages/components/image/stories/image.stories.tsx(2 hunks)packages/core/theme/src/components/image.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/components/image/src/image.tsx
- apps/docs/content/components/image/fallback.raw.jsx
- apps/docs/content/components/image/customLoading.raw.jsx
- packages/components/image/stories/image.stories.tsx
- packages/components/image/src/use-image.ts
🧰 Additional context used
🪛 Biome (1.9.4)
packages/components/image/__tests__/image.test.tsx
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 107-107: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🪛 LanguageTool
apps/docs/content/docs/components/image.mdx
[grammar] ~59-~59: Use proper spacing conventions.
Context: ...in case the loadingSrc is not defined. > Note: The URL uses `https://app.requ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~63-~63: Use proper spacing conventions.
Context: ...ly.io/delayto simulate a slow network. ### Custom Loading Image You can use thel...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~65-~65: Use proper spacing conventions.
Context: ... slow network. ### Custom Loading Image You can use the loadingSrc prop to dis...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~67-~67: Use proper spacing conventions.
Context: ...mage provided in src is still loading. > Note: The URL uses `https://app.requ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~71-~71: Use proper spacing conventions.
Context: ...ly.io/delayto simulate a slow network. ### Image with fallback You can use thefa...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~73-~73: Use proper spacing conventions.
Context: ...a slow network. ### Image with fallback You can use the fallbackSrc prop to di...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~75-~75: Use proper spacing conventions.
Context: ...prop to display a fallback image when: - ThefallbackSrc` prop is provided. - Th...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~77-~77: Use proper spacing conventions.
Context: ...: - The fallbackSrc prop is provided. - The image provided in src fails to loa...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~78-~78: Use proper spacing conventions.
Context: ...e image provided in src fails to load. - The image provided in src is not found...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~79-~79: Use proper spacing conventions.
Context: ...he image provided in src is not found. > Note: You can have both loadingSrc a...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~83-~83: Use proper spacing conventions.
Context: ...while loading and handling image errors. ### With Next.js Image Next.js provides an ...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
[grammar] ~101-~101: There might be a problem here.
Context: ...the "wrapper" only in the loading state. - fallbackImgWrapper: Applies styles to the the "wrapper" onl...
(QB_NEW_EN_MERGED_MATCH)
[grammar] ~102-~102: Avoid repeating words in a sentence.
Context: ...fallbackImgWrapper**: Applies styles to the the "wrapper" only in the failed state....
(QB_NEW_EN_OTHER_ERROR_IDS_000021)
[grammar] ~102-~102: Use proper spacing conventions.
Context: ... the "wrapper" only in the failed state. ## API ### Image Props <APITable data={...
(QB_NEW_EN_OTHER_ERROR_IDS_000007)
🔇 Additional comments (8)
packages/core/theme/src/components/image.ts (1)
40-41: LGTM! Clean addition of new theme slots.The new
loadingImgWrapperandfallbackImgWrapperslots follow the established pattern for theme slots. Empty strings are correct here since these slots will be populated with custom classes via theclassNamesprop at runtime.packages/components/image/__tests__/image.test.tsx (3)
7-8: LGTM! Well-defined test constants.The test constants are properly defined and will help maintain consistency across tests.
59-86: LGTM! Comprehensive test for loading state.The test properly verifies that:
- Loading image is displayed as background during loading
- Custom classes are applied correctly
- Loading background is removed after load
- onLoad callback is triggered
141-145: LGTM! Good test for baseline behavior.This test ensures the component works correctly when neither loading nor fallback behavior is defined.
apps/docs/content/docs/components/image.mdx (4)
59-72: LGTM! Clear documentation of new loading behavior.The documentation properly explains when the built-in skeleton is shown vs when custom loading images are used, and provides a helpful code demo.
75-84: LGTM! Improved clarity on fallback behavior.The documentation now clearly distinguishes between loading state (loadingSrc) and error state (fallbackSrc), removing previous confusion about when fallback applies.
166-176: LGTM! Comprehensive API documentation.The new loadingSrc prop is well-documented with clear description and proper typing. The updated fallbackSrc description also provides better clarity.
204-204: LGTM! Updated classNames type includes new slots.The classNames prop type correctly includes the new loadingImgWrapper and fallbackImgWrapper slots.
| beforeAll(() => { | ||
| function trackImageOnload() { | ||
| Object.defineProperty(window.Image.prototype, "onload", { | ||
| get() { | ||
| return this._onload; | ||
| }, | ||
| set(fn) { | ||
| imageOnload = fn; | ||
| this._onload = fn; | ||
| }, | ||
| configurable: true, | ||
| }); | ||
| } | ||
|
|
||
| trackImageOnload(); | ||
| }); | ||
|
|
||
| afterAll(() => { | ||
| // Restore original Image prototype | ||
| delete window.Image.prototype._onload; | ||
| Object.defineProperty(window.Image.prototype, "onload", { | ||
| value: null, | ||
| writable: true, | ||
| configurable: true, | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize cleanup by avoiding delete operator.
The global setup and teardown approach is excellent for consistency. However, the delete operator can impact performance.
Apply this diff to optimize the cleanup:
- delete window.Image.prototype._onload;
+ window.Image.prototype._onload = undefined;Also consider setting the descriptor to undefined instead of null:
- value: null,
+ value: undefined,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| beforeAll(() => { | |
| function trackImageOnload() { | |
| Object.defineProperty(window.Image.prototype, "onload", { | |
| get() { | |
| return this._onload; | |
| }, | |
| set(fn) { | |
| imageOnload = fn; | |
| this._onload = fn; | |
| }, | |
| configurable: true, | |
| }); | |
| } | |
| trackImageOnload(); | |
| }); | |
| afterAll(() => { | |
| // Restore original Image prototype | |
| delete window.Image.prototype._onload; | |
| Object.defineProperty(window.Image.prototype, "onload", { | |
| value: null, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); | |
| beforeAll(() => { | |
| function trackImageOnload() { | |
| Object.defineProperty(window.Image.prototype, "onload", { | |
| get() { | |
| return this._onload; | |
| }, | |
| set(fn) { | |
| imageOnload = fn; | |
| this._onload = fn; | |
| }, | |
| configurable: true, | |
| }); | |
| } | |
| trackImageOnload(); | |
| }); | |
| afterAll(() => { | |
| // Restore original Image prototype | |
| - delete window.Image.prototype._onload; | |
| + window.Image.prototype._onload = undefined; | |
| Object.defineProperty(window.Image.prototype, "onload", { | |
| - value: null, | |
| + value: undefined, | |
| writable: true, | |
| configurable: true, | |
| }); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 32-32: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🤖 Prompt for AI Agents
In packages/components/image/__tests__/image.test.tsx around lines 13 to 38,
avoid using the delete operator in the afterAll cleanup to improve performance.
Instead of deleting window.Image.prototype._onload, set it to undefined. Also,
when redefining the onload property descriptor, set its value to undefined
rather than null to better reset the property.
2 participants
Recreates the #3664 PR (fixes #3640).
Summary by CodeRabbit
New Features
Documentation
Tests
Chores