fix: added adapter to fix the SSR Icon missing error - #527
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe runtime plugin now configures Iconify with a request-aware fetch chain and uses it for custom icon loading. A Nuxt SSR fixture renders the Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
commit: |
| _api.setFetch((input, init) => { | ||
| const event = tryUseNuxtApp()?.ssrContext?.event as { fetch?: typeof globalThis.fetch } | undefined | ||
| const nitroFetch = (globalThis as typeof globalThis & { | ||
| $fetch?: { native?: typeof globalThis.fetch } | ||
| }).$fetch?.native | ||
|
|
||
| // Prefer request-aware fetch, but Nitro 2's useRequestFetch() has no `.native`. | ||
| // Its global native fetch keeps deferred relative requests local without retaining an event. | ||
| return (event?.fetch || nativeFetch || nitroFetch || globalThis.fetch)(input, init) | ||
| }) |
There was a problem hiding this comment.
I think this branch never actually runs, iconify does its fetching from a setTimeout so by the time this callback fires tryUseNuxtApp() returns undefined and it always ends up on globalThis.$fetch.native. What worked for me is grabbing the event at setup instead:
const event = import.meta.server ? useRequestEvent() : undefined
_api.setFetch(
event?.fetch
|| requestFetch.native
|| globalThis.$fetch?.native
|| globalThis.fetch,
)There was a problem hiding this comment.
Hey! Thank you for a review!
I double checked and yes, you are correct that it is a dead branch. On the other hand I think your setup grab theoritically can create another issue.
As the _api.setFetch() writes to Iconify's module-level variable the scheduled processes might call wrong event.fetch:
let fetchModule
function setFetch(fetch) {
fetchModule = fetch
}Consider the case:
- Request A runs plugin setup and installs
eventA.fetch. Iconify schedules A's icon request withsetTimeout - Request B runs plugin setup and installs
eventB.fetch - A's timer executes, but Iconify now calls
eventB.fetchinstead of correcteventA.fetch
While it would work in the most cases, it can forward wrong cookies, headers, middleware's context, etc.
I may be wrong though - I'm not proficient in nuxt codebases, so I would listen to your recommendations, but I thought that this should be mentioned before continuing.
If this is an issue though, I guess correct approach is just remove the event lookup and use native, as I haven't find approach how can we safely get event.fetch without changing Iconify's fetch module configuration.
Am I right to consider this an issue?
2 participants
🔗 Linked issue
Resolves #518.
📚 Description
After updating the
nuxt/iconup from@2.3.1the icons fail to render if app uses SSR. For ex:Related issue describes the core regression correctly.
Solution
The simple priority logic to pick correcrt
fetchwas added to resolve both initial issue #514 (where regression appeared) and remove that regression.Here's the logic behind the prioritization:
event.fetchif it is available as the most "context-rich": relative Nitro routes, preserving request’s headers and context, base URL, etc.useRequestFetch().nativeif it is available. Used in browser (or future Nuxt 5) and keeping fix by fix: avoid relying on global fetch #514.Here's the regression, as Nuxt 4/Nitro 2
useRequestFetch()doesn't expose.native.globalThis.$fetch.nativeas the Nuxt 4/Nitro 2 SSR compatibility path.globalThis.fetchas the final safety net.I also added fixture, expanded smoke and wrote regression tests.