feat: create prefetch functions for Tanstack Query plugin #1715
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces significant enhancements to the Changes
Possibly related PRs
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 (
|
|
Hey @Arkanii , thanks for making this PR. I'll follow up here a bit later. |
Hi @Arkanii , my apologies for the late response. Just to clarify your goal here. Do you want to do server-side prefetch with SvelteKit? If so, maybe we should simply generate a set of |
Hello ! No problem. 😄 I'll try to check your idea a lit later. |
|
Mmmh I think it's too complicated for me to end this... |
No worries @Arkanii , I'll follow up on this PR! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (3)
packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (1)
119-120: ClarifyoptimisticUpdateLogicIn constructing the
queryKey, theoptimisticUpdateproperty is set using:optimisticUpdate: options?.optimisticUpdate !== false,This logic means that
optimisticUpdatedefaults totrueunless explicitly set tofalse. While functional, this can be confusing.Consider simplifying the logic for clarity:
- optimisticUpdate: options?.optimisticUpdate !== false, + optimisticUpdate: options?.optimisticUpdate ?? true,This change makes it clear that if
optimisticUpdateisundefined, it defaults totrue.packages/plugins/tanstack-query/tests/plugin.test.ts (1)
229-229: Avoid usinglatestin dependency versionsIn the
extraDependencies, you have specified@tanstack/vue-query@latest. Usinglatestcan lead to unpredictable builds if the package is updated. Consider pinning to a specific version to ensure consistent and reliable builds.packages/plugins/tanstack-query/src/generator.ts (1)
28-29: Consider Refactoring into a Class for Better Parameter ManagementThe TODO comment suggests turning the code into a class to simplify parameter passing. Refactoring into a class can enhance code organization, maintainability, and readability by encapsulating related functionalities and reducing the need for passing multiple parameters through functions.
Would you like assistance in refactoring this code into a class structure?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/plugins/tanstack-query/src/generator.ts (17 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/react.ts (3 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (5 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/vue.ts (5 hunks)
- packages/plugins/tanstack-query/tests/plugin.test.ts (7 hunks)
🧰 Additional context used
🪛 Biome
packages/plugins/tanstack-query/src/runtime-v5/vue.ts
[error] 40-40: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (18)
packages/plugins/tanstack-query/src/runtime-v5/svelte.ts (3)
110-128: Avoid Unnecessary Type Assertions inprefetchModelQueryIn the
prefetchModelQueryfunction, when callingqueryClient.prefetchQuery, you're spreading...optionswithout the need for a type assertion. This is good practice. However, please ensure that the types ofoptionsalign correctly with the expected parameters to maintain type safety and avoid potential runtime errors.
Line range hint
282-298: VerifygetContextUsage for SSR CompatibilityThe
useModelMutationfunction usesgetContextto access theloggingconfiguration:const { logging } = getContext<APIContext>(SvelteQueryContextKey);Since
getContextis a Svelte function that may not be available during server-side rendering (SSR), this could cause issues when performing mutations on the server.Run the following script to check for
getContextusage and assess its compatibility with SSR:#!/bin/bash # Description: Find all usages of getContext in mutation functions rg --type typescript 'getContext<APIContext>'Ensure that
getContextis not called during SSR or provide alternative methods for accessing context data when on the server. Consider injecting the necessary context or using a different approach that is SSR-safe.
111-117: Ensure Safe Handling of OptionalfetchParameterIn the
prefetchModelQueryfunction, thefetchparameter is optional. Verify that thefetcherfunction can handle anundefinedfetchparameter without causing errors.Run the following script to check where
fetcheris used and ensure it handlesundefinedsafely:Ensure that all code paths in
fetcheraccount forfetchbeingundefinedand provide appropriate fallbacks or error handling.packages/plugins/tanstack-query/tests/plugin.test.ts (7)
79-104: Well-structured function to generate prefetch source filesThe
makePrefetchSourcefunction is properly implemented to create prefetch source files for different targets, enhancing the plugin's capabilities.
135-135: Enabling prefetch generation in React plugin configurationSetting
generatePrefetch = truein the plugin configuration correctly enables the prefetch functions for React v5.
162-162: Verify the inclusion ofsuspense.tsin Vue and Svelte testsThe
suspense.tssource file is included in the React plugin tests to cover Suspense features but is not included in the Vue and Svelte plugin tests. If Suspense is also supported in Vue and Svelte, consider adding similar test cases for consistency.
226-226: Enabling prefetch generation in Vue plugin configurationSetting
generatePrefetch = truein the plugin configuration correctly enables the prefetch functions for Vue v5.
237-237: Including prefetch source files in Vue testsAdding
makePrefetchSource('vue')toextraSourceFilescorrectly includes the prefetch source files in the Vue tests.
301-301: Enabling prefetch generation in Svelte plugin configurationSetting
generatePrefetch = truein the plugin configuration correctly enables the prefetch functions for Svelte v5.
312-312: Including prefetch source files in Svelte testsAdding
makePrefetchSource('svelte')toextraSourceFilescorrectly includes the prefetch source files in the Svelte tests.packages/plugins/tanstack-query/src/runtime-v5/react.ts (1)
Line range hint
84-250: Verify consistency across different frameworksGiven the PR's objective to implement prefetch functions across React, Vue, and Svelte, ensure that similar functions are implemented in the corresponding files for Vue and Svelte. This maintains feature parity across all supported frameworks.
To check for the existence of similar implementations in Vue and Svelte, you can run:
✅ Verification successful
Consistency across different frameworks verified successfully.
All prefetch functions are implemented in Vue and Svelte runtime files, ensuring feature parity across React, Vue, and Svelte.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that prefetch functions are implemented in Vue and Svelte runtime files. # Search for 'prefetchModelQuery' in Vue files fd --type f 'vue.ts' | xargs rg 'export function prefetchModelQuery' # Search for 'prefetchModelQuery' in Svelte files fd --type f 'svelte.ts' | xargs rg 'export function prefetchModelQuery'Length of output: 381
packages/plugins/tanstack-query/src/runtime-v5/vue.ts (5)
8-11: Addition of necessary imports for query handlingThe imports of
FetchInfiniteQueryOptions,FetchQueryOptions, andQueryClientfrom@tanstack/vue-queryare appropriate and required for the new functions.
107-137: New functionprefetchModelQueryenhances prefetching capabilitiesThe
prefetchModelQueryfunction is correctly implemented, allowing prefetching of queries using theQueryClient. This addition adheres to best practices and improves the library's functionality.
149-169: New functionfetchModelQueryfor fetching queriesThe
fetchModelQueryfunction is appropriately implemented, enabling fetching of queries with theQueryClient. The implementation is correct and aligns with the expected usage.
219-240: New functionprefetchInfiniteModelQueryfor infinite queriesThe
prefetchInfiniteModelQueryfunction effectively adds prefetching capabilities for infinite queries. The implementation is correct and follows best practices.
252-273: New functionfetchInfiniteModelQueryadded for infinite queriesThe
fetchInfiniteModelQueryfunction is correctly implemented, allowing fetching of infinite queries using theQueryClient. This enhances the library's functionality for handling infinite queries.packages/plugins/tanstack-query/src/generator.ts (2)
45-55: LGTM: Error Handling Logic is CorrectThe added error handling correctly validates the
generatePrefetchoption's type and ensures compatibility with version 'v4'. This enhances the robustness of the code by providing clear error messages to the user.
183-234: Ensure Server-Side Compatibility in Prefetch FunctionsThe addition of prefetch functions enhances functionality, but ensure that they are compatible with server-side execution across all supported frameworks. Particular attention is needed for Svelte, where certain client-side APIs may not be available server-side.
Consider reviewing the implementation to ensure that all server-side requirements are met and that framework-specific nuances are handled appropriately.
You can use the following script to identify potential client-side API usage in prefetch functions:
#!/bin/bash # Description: Search for client-side APIs used in prefetch functions that may not be server-compatible. # Expected: Prefetch functions should not use client-side only APIs like getHooksContext. rg --type ts "getHooksContext|getContext" -A 5
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (7)
packages/plugins/tanstack-query/src/runtime/index.ts (2)
Line range hint
1-9: LGTM! Consider adding documentation for new exports.The additions of
DEFAULT_QUERY_ENDPOINTandExtraPrefetchOptionsalign well with the PR objectives of implementing server-side prefetching. These new exports will likely enhance the plugin's prefetching capabilities.Consider adding inline documentation (JSDoc comments) for the new exports to explain their purpose and usage, especially for
ExtraPrefetchOptions. This would help developers understand how to use these new features in the context of server-side prefetching.
Line range hint
1-9: Summary: Good progress on server-side prefetching implementationThe changes in this file are a step in the right direction for implementing server-side prefetching. The addition of
DEFAULT_QUERY_ENDPOINTandExtraPrefetchOptionsprovides the necessary building blocks for this functionality.Regarding the challenges mentioned in the PR objectives:
For the Svelte-specific issue with
getContext(...), this file doesn't directly address it. You might need to look into creating a Svelte-specific implementation that doesn't rely ongetContextfor server-side operations.For testing across Vue and React frameworks, these changes should be framework-agnostic, which is good. However, you'll need to ensure that the implementation of these new exports in the
./commonfile works correctly across all frameworks.Consider creating separate files for framework-specific implementations (e.g.,
svelte.ts,react.ts,vue.ts) that utilize these common exports. This could help address the Svelte-specific issues while maintaining a consistent API across frameworks.Would you like assistance in creating these framework-specific files or in implementing the server-side prefetching logic using these new exports?
packages/plugins/tanstack-query/src/runtime/common.ts (1)
113-116: LGTM! Consider adding a brief comment.The new
ExtraPrefetchOptionstype is a good addition, aligning well with the PR's objective of implementing server-side prefetching. It's correctly defined as a subset ofAPIContext, focusing on the essential properties for prefetching.Consider adding a brief comment explaining the purpose of this type, similar to the comments for other type definitions in this file. For example:
/** * Extra options for prefetching queries. */ export type ExtraPrefetchOptions = Pick<APIContext, 'endpoint' | 'fetch'>;packages/plugins/tanstack-query/src/generator.ts (2)
Line range hint
87-234: LGTM with suggestion: Consider refactoring for improved readabilityThe changes to
generateQueryHookcorrectly implement the new functionality for prefetching and nullable types. However, the function has grown quite large and complex.Consider refactoring this function into smaller, more focused functions to improve readability and maintainability. For example, you could extract the prefetch hook generation into a separate function.
Here's a suggestion for refactoring:
function generatePrefetchHooks(sf: SourceFile, model: string, operation: string, capOperation: string, argsType: string, inputType: string, returnType: string, supportInfinite: boolean, overrideTypeParameters?: string[]) { const modes = [ { mode: 'prefetch', infinite: false }, { mode: 'fetch', infinite: false }, ]; if (supportInfinite) { modes.push({ mode: 'prefetch', infinite: true }, { mode: 'fetch', infinite: true }); } for (const { mode, infinite } of modes) { // ... (rest of the prefetch hook generation logic) } } function generateQueryHook( // ... (existing parameters) ) { // ... (existing logic for non-prefetch hooks) if (generatePrefetch) { generatePrefetchHooks(sf, model, operation, capOperation, argsType, inputType, returnType, supportInfinite, overrideTypeParameters); } }This refactoring would make the
generateQueryHookfunction more manageable and easier to understand.
Line range hint
768-779: LGTM with suggestion: Consider adding explicit return typeThe changes to
makeQueryArgsTypecorrectly handle the different requirements for prefetch and non-prefetch scenarios in Vue. This is a good improvement for type safety.To further enhance clarity and type safety, consider adding an explicit return type to the function:
function makeQueryArgsType(target: string, argsType: string, prefetch: boolean): string { // ... (existing implementation) }This will make the function's contract clearer and help catch any potential type-related issues earlier.
packages/plugins/tanstack-query/src/runtime-v5/vue.ts (2)
Line range hint
188-198: Investigate and resolve typing issues inuseInfiniteModelQueryThe usage of
anyforqueryOptionssuggests there are typing issues withvue-query'suseInfiniteQuery. This reduces type safety and may mask potential errors. Consider reviewing the typings or updating the dependencies to address this issue.I can help investigate the typing issues with
useInfiniteQueryto find a solution that maintains type safety without resorting toany.
Line range hint
282-298: Resolve the typing issue to eliminateanyinuseModelMutationThe use of
anyforfinalOptionsdue to typing problems reduces type safety. Addressing the typing issues will improve code reliability and maintain the benefits of TypeScript's type checking.I noticed the TODO comment about the typing problem. Would you like assistance in refining the types to eliminate the use of
any?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- packages/plugins/tanstack-query/src/generator.ts (16 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/index.ts (1 hunks)
- packages/plugins/tanstack-query/src/runtime-v5/vue.ts (5 hunks)
- packages/plugins/tanstack-query/src/runtime/common.ts (1 hunks)
- packages/plugins/tanstack-query/src/runtime/index.ts (1 hunks)
🧰 Additional context used
🪛 Biome
packages/plugins/tanstack-query/src/runtime-v5/vue.ts
[error] 40-40: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (6)
packages/plugins/tanstack-query/src/runtime-v5/index.ts (1)
Line range hint
1-9: LGTM! New exports align with PR objectives.The addition of
DEFAULT_QUERY_ENDPOINTandExtraPrefetchOptionsto the exports is consistent with the PR's goal of implementing server-side prefetching for queries. These new exports will likely be used in the prefetching implementation across different frameworks.To ensure the correct implementation of these new exports, please run the following script:
This will help confirm that these exports are properly defined in the common runtime file.
✅ Verification successful
LGTM! Verified the implementation of new exports.
The definitions for
DEFAULT_QUERY_ENDPOINTandExtraPrefetchOptionsare correctly implemented incommon.ts, aligning with the PR's objectives for server-side prefetching.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of new exports in the common runtime file # Test: Check the implementation of DEFAULT_QUERY_ENDPOINT and ExtraPrefetchOptions rg --type typescript -A 5 'DEFAULT_QUERY_ENDPOINT|ExtraPrefetchOptions' packages/plugins/tanstack-query/src/runtime/common.tsLength of output: 162
Script:
#!/bin/bash # Description: Verify the definitions of DEFAULT_QUERY_ENDPOINT and ExtraPrefetchOptions in the common runtime file # Check for DEFAULT_QUERY_ENDPOINT definition rg 'export\s+const\s+DEFAULT_QUERY_ENDPOINT' packages/plugins/tanstack-query/src/runtime/common.ts # Check for ExtraPrefetchOptions type definition rg 'export\s+type\s+ExtraPrefetchOptions' packages/plugins/tanstack-query/src/runtime/common.tsLength of output: 325
packages/plugins/tanstack-query/src/runtime/common.ts (2)
Line range hint
1-478: Overall assessment: LGTMThe addition of the
ExtraPrefetchOptionstype is a minimal and non-disruptive change that aligns well with the PR's objective of implementing server-side prefetching. It follows the existing coding style and naming conventions in the file.To fully validate this change:
- Ensure that the
ExtraPrefetchOptionstype is used effectively in the implementation of prefetching functionality in other files.- Confirm that the prefetching implementation addresses the challenges mentioned in the PR objectives, particularly regarding the use of
getHooksContextand the Svelte-specific issues.
113-116: Verify usage ofExtraPrefetchOptionsin other files.The new
ExtraPrefetchOptionstype is well-defined and exported, but it's not used within this file. To ensure its effectiveness and necessity:
- Confirm that this type is used in the implementation of prefetching functionality in other files.
- Verify that the selected properties (
endpointandfetch) are sufficient for the prefetching requirements.To check the usage of
ExtraPrefetchOptionsacross the codebase, run the following script:✅ Verification successful
ExtraPrefetchOptions is properly utilized across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of ExtraPrefetchOptions in other files # Search for imports of ExtraPrefetchOptions echo "Files importing ExtraPrefetchOptions:" rg "import.*ExtraPrefetchOptions" --type ts # Search for usage of ExtraPrefetchOptions echo "\nUsage of ExtraPrefetchOptions:" rg "ExtraPrefetchOptions" --type tsLength of output: 895
packages/plugins/tanstack-query/src/generator.ts (3)
45-54: LGTM: Proper handling of the newgeneratePrefetchoptionThe changes correctly validate the
generatePrefetchoption and ensure it's only used with version "v5". This is a good addition to maintain compatibility and prevent misuse.
828-838: LGTM: Well-implementedmakePrefetchQueryOptionsfunctionThe new
makePrefetchQueryOptionsfunction is a good addition. It correctly handles both infinite and non-infinite queries, and includes the appropriate extra options. The function is concise and easy to understand.
421-422: LGTM: Consistent implementation of prefetch functionalityThe minor changes throughout the file, including updates to various functions to include the
generatePrefetchparameter and the addition of new import statements, are consistent with the overall addition of prefetch functionality. These changes appear to be correctly implemented and maintain the coherence of the codebase.Also applies to: 429-429, 709-725, 747-748
|
Hi @Arkanii , I've made some progress to change the PR to generate However, I believe more though and work is needed about how data is actually fetched from the server side. It's complicated and not performant if the hooks still send an HTTP request. It should probably just directly call enhanced prisma client. May I keep the PR open for now and continue thinking about this? I really appreciate that you started this effort. It's been a long-waited feature! |
Sure no problem ! I'll just put it in draft. |
|
ops sorry @ymc9 I closed the PR... |
Hello!
I need help with this one. 😓
I'm trying to run this on Svelte but prefetch queries need to be executed on the server so I can't use the
getHooksContextfunction and I can't find any solution for it yet.I also need to test on Vue and React. If anyone can test it that would be great as I don't have much time to test on all three frameworks. 🙏🏻
Thanks !