fix(use-image): image ReferenceError in SSR #4122
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
🦋 Changeset detectedLatest commit: ac3e063 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
WalkthroughThis pull request introduces a patch for the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (3)
.changeset/wild-jobs-explain.md (1)
5-5: Consider enhancing the changeset descriptionWhile accurate, the description could be more informative for future reference. Consider adding:
- The specific ReferenceError being fixed
- The technical solution implemented (e.g., changing from
new Image()todocument.createElement("img"))- The impact on SSR applications
-fix Image ReferenceError in SSR +fix: Image ReferenceError in SSR by using document.createElement instead of new Image() + +Resolves ReferenceError when creating image elements during server-side rendering by +switching from `new Image()` to `document.createElement("img")`. This change ensures +proper image handling in SSR environments.packages/hooks/use-image/src/index.ts (2)
Line range hint
52-63: Consider using useEffect for cleanupFor better SSR compatibility, consider moving the cleanup logic to
useEffect. WhileuseSafeLayoutEffectprovides some safety,useEffectis more appropriate for side effects that don't need to be synchronous with layout updates.- useSafeLayoutEffect(() => { + useEffect(() => { if (firstMount.current) { firstMount.current = false; return; } setStatus(setImageAndGetInitialStatus(props, imageRef)); return () => { flush(); }; }, [src, crossOrigin, srcSet, sizes, loading]);
Line range hint
42-42: Improve type safety for imageRefConsider making the null case explicit in the type definition of
imageReffor better type safety, especially in SSR contexts.- const imageRef = useRef<HTMLImageElement | null>(); + const imageRef = useRef<HTMLImageElement | null>(null);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.changeset/wild-jobs-explain.md(1 hunks)packages/hooks/use-image/src/index.ts(1 hunks)
🔇 Additional comments (1)
.changeset/wild-jobs-explain.md (1)
1-3: LGTM on the patch version bump!
The patch version bump is appropriate for this bug fix as it maintains backward compatibility while addressing the SSR-related ReferenceError.
| @@ -122,7 +122,7 @@ function setImageAndGetInitialStatus( | |||
| if (!src) return "pending"; | |||
| if (ignoreFallback) return "loaded"; | |||
|
|
|||
| const img = new Image(); | |||
| const img = document.createElement("img"); | |||
There was a problem hiding this comment.
Add SSR safety check for document access
While using document.createElement("img") fixes the Image constructor ReferenceError, accessing document directly can still cause SSR issues. Consider adding a check for the browser environment.
- const img = document.createElement("img");
+ const img = typeof document !== "undefined"
+ ? document.createElement("img")
+ : null;
+
+ if (!img) return "pending";📝 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.
| const img = document.createElement("img"); | |
| const img = typeof document !== "undefined" | |
| ? document.createElement("img") | |
| : null; | |
| if (!img) return "pending"; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/hooks/use-image/__tests__/use-image.test.tsx (1)
32-35: Consider adjusting error simulation timingThe error is simulated before the hook renders, which might not catch issues that occur during image loading. Consider moving the error simulation after the initial render:
-mockImage.simulate("error"); const {result} = renderHook(() => useImage({src: "/test.png"})); expect(result.current).toEqual("loading"); +mockImage.simulate("error"); await waitFor(() => expect(result.current).toBe("failed"));
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/hooks/use-image/__tests__/use-image.test.tsx(2 hunks)
🔇 Additional comments (4)
packages/hooks/use-image/__tests__/use-image.test.tsx (4)
1-1: LGTM! Good modernization of test utilities
The switch to @testing-library/react and addition of waitFor follows current best practices for testing React hooks.
17-19: LGTM! Clean test refactoring
The destructured assignment improves readability while maintaining the same test behavior.
23-27: Consider adding SSR-specific test cases
While the current test changes look good, given that this PR fixes an SSR-related ReferenceError, we should add test coverage for SSR scenarios to prevent regression.
Would you like help creating SSR-specific test cases?
Line range hint 1-37: Review overall test coverage
A few observations about the test suite:
- The cached image test case was removed - was this intentional?
- Missing test coverage for SSR scenarios, which is crucial given the PR's focus on fixing SSR-related issues
- No tests verifying the switch from
new Image()todocument.createElement("img")
Closes #
📝 Description
applied the same PR from beta to canary.
⛳️ Current behavior (updates)
🚀 New behavior
💣 Is this a breaking change (Yes/No):
📝 Additional Information
Summary by CodeRabbit
Bug Fixes
New Features
useImagehook, streamlining how images are managed and potentially enhancing performance.Tests
useImagehook, updating methodologies for handling asynchronous state changes.