fix(nuxt): prefer accessing globalThis over window
#33125
Conversation
|
|
WalkthroughClient-side references to the global environment were changed from Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes ✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
@nuxt/kit
nuxt
@nuxt/rspack-builder
@nuxt/schema
@nuxt/vite-builder
@nuxt/webpack-builder
commit: |
![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 (3)
packages/nuxt/src/app/compat/interval.ts (3)
6-6: Optional: cast to DOM typing to avoid NodeJS.Timeout bleed-throughIn repos that include @types/node alongside DOM libs, global typings can merge and surface NodeJS.Timeout at call sites. Casting the client branch keeps the familiar DOM number return in editors/TS tooling.
Apply:
-export const setInterval = import.meta.client - ? globalThis.setInterval +export const setInterval = import.meta.client + ? (globalThis.setInterval as typeof window.setInterval) : () => {
3-3: Nit: add a brief JSDoc noting client-only usageA one-liner helps consumers understand the server behaviour quickly.
-const intervalError = '[nuxt] `setInterval` should not be used on the server. Consider wrapping it with an `onNuxtReady`, `onBeforeMount` or `onMounted` lifecycle hook, or ensure you only call it in the browser by checking `import.meta.client`.' +/** Client-only alias for setInterval; throws in dev and logs in prod on the server. */ +const intervalError = '[nuxt] `setInterval` should not be used on the server. Consider wrapping it with an `onNuxtReady`, `onBeforeMount` or `onMounted` lifecycle hook, or ensure you only call it in the browser by checking `import.meta.client`.'
5-16: Audit complete—no window. timers found, but NodeJS.Timeout types present*
- No other
window.setTimeout/setIntervalor manual shims detected beyondpackages/nuxt/src/app/compat/interval.ts.- In
cookie.ts,loading-indicator.tsandfetch.ts, timer returns are typed asNodeJS.Timeout—consider replacing withnumberor defining a sharedTimeralias underapp/compatfor consistency and browser compatibility.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/nuxt/src/app/compat/interval.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/app/compat/interval.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (1)
packages/nuxt/src/app/compat/interval.ts (1)
5-7: Use of globalThis fixes Worker context — LGTMSwitching from window.setInterval to globalThis.setInterval correctly handles Web Workers while preserving the server guard. Good, targeted fix.
danielroe
changed the title
setInterval from globalThisglobalThis over window
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/nuxt/src/app/composables/cookie.ts (2)
174-180: Guard document access for Worker contextsuseCookie currently throws in Web Workers because readRawCookies touches document.cookie when import.meta.client is true. Add a runtime guard.
Apply:
- } else if (import.meta.client) { - return parse(document.cookie, opts) + } else if (import.meta.client) { + if (typeof document !== 'undefined') { + return parse(document.cookie, opts) + } + // In non-Window clients (e.g. Web Workers) document is unavailable + return undefined }
189-193: Mirror the Worker guard when writing cookiesProtect writeClientCookie with the same document check to avoid Worker runtime errors if useCookie is used there.
Apply:
- if (import.meta.client) { - document.cookie = serializeCookie(name, value, opts) - } + if (import.meta.client && typeof document !== 'undefined') { + document.cookie = serializeCookie(name, value, opts) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/nuxt/src/app/composables/cookie.ts(1 hunks)packages/nuxt/src/app/composables/url.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Follow standard TypeScript conventions and best practices
Files:
packages/nuxt/src/app/composables/cookie.tspackages/nuxt/src/app/composables/url.ts
🧠 Learnings (1)
📓 Common learnings
Learnt from: GalacticHypernova
PR: nuxt/nuxt#26468
File: packages/nuxt/src/components/plugins/loader.ts:24-24
Timestamp: 2024-11-05T15:22:54.759Z
Learning: In `packages/nuxt/src/components/plugins/loader.ts`, the references to `resolve` and `distDir` are legacy code from before Nuxt used the new unplugin VFS and will be removed.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: code
🔇 Additional comments (2)
packages/nuxt/src/app/composables/url.ts (1)
9-10: Worker-safe global: LGTMSwitching to globalThis avoids Worker crashes and preserves behaviour.
packages/nuxt/src/app/composables/cookie.ts (1)
36-37: Worker-safe cookieStore access: LGTMUsing globalThis.cookieStore aligns with the PR goal and gracefully falls back to BroadcastChannel when unavailable.
CodSpeed Performance ReportMerging #33125 will not alter performanceComparing Summary
|
1 participant
🔗 Linked issue
resolves #33124
📚 Description
we do not always have
windowglobally availableI'll investigate also to see if there's anything else we can helpfully swap out for web workers