fix(hooks): prefer to use global fetch when available #1826
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to multiple files across different packages. The Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🔇 Additional comments (2)packages/plugins/swr/src/runtime/index.ts (2)
The rename to
Add error handling for cross-fetch import While the comment about Cloudflare Workers compatibility is helpful, the cross-fetch import still needs proper error handling to prevent runtime issues if the module is not available. Apply this diff to add error handling: const _fetch =
customFetch ??
// check if fetch is available globally
(typeof fetch === 'function'
? fetch
: // fallback to 'cross-fetch' if otherwise
- (await import('cross-fetch')).default);
+ await import('cross-fetch')
+ .then((module) => module.default)
+ .catch((error) => {
+ console.error('Failed to import cross-fetch:', error);
+ throw new Error('No fetch implementation available. Please provide a custom fetch function.');
+ }));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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/plugins/tanstack-query/src/runtime/common.ts (1)
138-146: LGTM! Consider enhancing error handling for dynamic import.The implementation correctly prioritizes fetch sources and includes helpful compatibility notes. The logic is clean and well-structured.
Consider wrapping the dynamic import in a try-catch to handle potential module loading failures:
const _fetch = customFetch ?? // check if fetch is available globally (typeof fetch === 'function' ? fetch : // fallback to 'cross-fetch' if otherwise - (await import('cross-fetch')).fetch); + // Safely handle dynamic import failures + await import('cross-fetch').catch(err => { + console.error('Failed to load cross-fetch:', err); + throw new Error('No fetch implementation available'); + }).then(mod => mod.fetch));packages/plugins/swr/src/runtime/index.ts (1)
378-390: Optimize performance by caching the imported 'cross-fetch' moduleThe dynamic import of 'cross-fetch' within the
fetcherfunction may introduce performance overhead if invoked multiple times. Consider caching the imported module to avoid repeated dynamic imports.Apply this diff to cache the imported module:
+let cachedCrossFetch: FetchFn | null = null; export async function fetcher<R, C extends boolean>( url: string, options?: RequestInit, customFetch?: FetchFn, checkReadBack?: C ): Promise<C extends true ? R | undefined : R> { const _fetch = customFetch ?? // check if fetch is available globally (typeof fetch === 'function' ? fetch : // fallback to 'cross-fetch' if otherwise - await import('cross-fetch') - .then((module) => module.default) - .catch((error) => { - console.error('Failed to import cross-fetch:', error); - throw new Error('No fetch implementation available. Please provide a custom fetch function.'); - })); + cachedCrossFetch ?? + (cachedCrossFetch = await import('cross-fetch') + .then((module) => module.default) + .catch((error) => { + console.error('Failed to import cross-fetch:', error); + throw new Error('No fetch implementation available. Please provide a custom fetch function.'); + }))); const res = await _fetch(url, options); // ... rest of the function ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (13)
package.jsonis excluded by!**/*.jsonpackages/ide/jetbrains/package.jsonis excluded by!**/*.jsonpackages/language/package.jsonis excluded by!**/*.jsonpackages/misc/redwood/package.jsonis excluded by!**/*.jsonpackages/plugins/openapi/package.jsonis excluded by!**/*.jsonpackages/plugins/swr/package.jsonis excluded by!**/*.jsonpackages/plugins/tanstack-query/package.jsonis excluded by!**/*.jsonpackages/plugins/trpc/package.jsonis excluded by!**/*.jsonpackages/runtime/package.jsonis excluded by!**/*.jsonpackages/schema/package.jsonis excluded by!**/*.jsonpackages/sdk/package.jsonis excluded by!**/*.jsonpackages/server/package.jsonis excluded by!**/*.jsonpackages/testtools/package.jsonis excluded by!**/*.json
📒 Files selected for processing (3)
packages/ide/jetbrains/build.gradle.kts(1 hunks)packages/plugins/swr/src/runtime/index.ts(1 hunks)packages/plugins/tanstack-query/src/runtime/common.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/ide/jetbrains/build.gradle.kts
🔇 Additional comments (1)
packages/plugins/tanstack-query/src/runtime/common.ts (1)
135-135: LGTM! Note: Breaking change in parameter name.
Good rename from fetch to customFetch to avoid shadowing the global fetch. However, this is a breaking change that should be noted in the changelog.
Let's verify the impact of this breaking change:
No description provided.