Skip to content

Conversation

gterral
Copy link

@gterral gterral commented Apr 20, 2023

This pull request updates the prefer-object-syntax eslint rule to add support for the fetchQuery method

I tried to follow the Contributing guide, please let me know if I need to change something.

Related discussion => #5300

Thank you for the awesome library ❤️

@vercel
Copy link

vercel bot commented Apr 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Apr 28, 2023 9:00am

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 20, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 47ef424:

Sandbox Source
@tanstack/query-example-react-basic-typescript Configuration
@tanstack/query-example-solid-basic-typescript Configuration
@tanstack/query-example-svelte-basic Configuration
@tanstack/query-example-vue-basic Configuration

@@ -7,6 +7,7 @@ import { objectKeys } from '../../utils/object-utils'
const QUERY_CALLS = {
useQuery: { key: 'queryKey', fn: 'queryFn', type: 'query' },
createQuery: { key: 'queryKey', fn: 'queryFn', type: 'query' },
fetchQuery: { key: 'queryKey', fn: 'queryFn', type: 'query' },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the difference is: fetchQuery comes from queryClient, so it would need to be queryClient.fetchQuery. I'd want to avoid false-positives from other, random methods called fetchQuery. @Newbie012 FYI

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your answer @TkDodo !

I could write the eslint rule to use the Identifier node listener instead, but indeed we might encounter some false positives if someone handles functions also called fetchQuery :/

I could do a rule on queryClient.fetchQuery, but maybe they use a different variable name for their queryClient 😅

I am not sure what's the best way to implement this rule then. I will try to think about it ! Thanks again!

Copy link
Collaborator

@Newbie012 Newbie012 Apr 26, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rule was initially written in mind to support only useQuery. Since then, other hooks needed to be supported, so I refactored the rule in a way of identifying only hooks, which is not the case for queryClient.fetchQuery. The identification of fetchQuery should be different than the other ways:

  1. Look for fetchQuery call expression.
  2. Track down its source and check if it came from useQueryClient (I already wrote a function that tells if a given reference is a queryClient. It's not exactly the same issue as here, but it's a good starting point):

isQueryClientReference(reference: TSESLint.Scope.Reference) {
const declarator = reference.resolved?.defs[0]?.node
return (
declarator?.type === AST_NODE_TYPES.VariableDeclarator &&
declarator.init?.type === AST_NODE_TYPES.CallExpression &&
declarator.init.callee.type === AST_NODE_TYPES.Identifier &&
declarator.init.callee.name === 'useQueryClient'
)
},

  1. Apply rule checks if positive.

This way you don't have any false positives. Doesn't matter if you have a different name than queryClient or destructure fetchQuery from it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very clear thanks! I will try to give it a try in the upcoming days!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gterral any updates here? This is pretty stale and I'd close it if you're not working on this anymore, since the rule won't exist in v5.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, sorry didn't took the time to work on it yet as this wasn't really urgent on my side.
(And just noticed it's been almost 2 months already 😱).

I don't think I will able to work on it during the upcoming week, so I can understand if you prefer to close the issue rather keeping it open, but if you do let it open, I promise that I will do it at one point!

Hopefully in the 3/4 weeks. (July should be quieter on my side).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it could very well be that we have v5 out by then 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok!

Sorry and good luck for the v5 release then ;)!

@nx-cloud
Copy link

nx-cloud bot commented Apr 21, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 47ef424. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 4 targets

Sent with 💌 from NxCloud.

@codecov-commenter
Copy link

Codecov Report

Patch and project coverage have no change.

Comparison is base (058f030) 97.12% compared to head (99f31fa) 97.12%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #5301   +/-   ##
=======================================
  Coverage   97.12%   97.12%           
=======================================
  Files          11       11           
  Lines         383      383           
  Branches      140      140           
=======================================
  Hits          372      372           
  Misses         11       11           
Impacted Files Coverage Δ
...-query-object-syntax/prefer-query-object-syntax.ts 96.19% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants