Skip to content

Conversation

@Newbie012
Copy link
Collaborator

@Newbie012 Newbie012 commented Oct 23, 2022

This PR contains two projects:

  • an ESLint plugin
  • an example project that uses the plugin

A few notes:

  • This PR support (currently) only array expression of queryKey (i.e, query key factory pattern is not supported).
  • I wasn't quite sure what's the preferences for building a production/dev build, so I stuck with my original setup (tsup)

@Newbie012 Newbie012 force-pushed the feature-eslint-plugin branch from 2905682 to a341931 Compare October 23, 2022 17:56
@codesandbox-ci
Copy link

codesandbox-ci bot commented Oct 23, 2022

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 25b969b:

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

Copy link
Collaborator

@TkDodo TkDodo left a comment

Choose a reason for hiding this comment

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

This is very good work, thank you ❤️

What we would need to add is some docs for the eslint plugin, how to download it, how to use it, what the rule does etc.

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 23, 2022

also, @KubaJastrz I really like your eslint-plugin-react-query, especially this rule:

https://github.com/KubaJastrz/eslint-plugin-react-query/blob/master/docs/rules/prefer-query-object-syntax.md

So if you don't object, I would eventually like to add that one here, too :)

@KubaJastrz
Copy link

@TkDodo I'm okay with that, let me know if you want to merge projects, as I'm no longer actively maintaining it.

Let me know if you need npm package name too.

@Newbie012
Copy link
Collaborator Author

Additional improvements:

  • Works with @babel/eslint-parser (the default parser).
  • Automatically whitelists call expressions (e.g fetch(...), axios.get(...), api.entity.get(...)).

@Newbie012 Newbie012 marked this pull request as ready for review October 23, 2022 23:50
@Newbie012
Copy link
Collaborator Author

I'm not exactly sure why the CI is failing

@TkDodo
Copy link
Collaborator

TkDodo commented Oct 25, 2022

I'm not exactly sure why the CI is failing

eslint is failing:

packages/eslint-plugin-query test:eslint:   0:0  error  Parsing error: Cannot read file '/home/runner/work/query/query/packages/eslint-plugin-query/tsconfig.base.json'

tests are flaky so I re-ran them. I think this got worse since we parallelized the pipeline, not sure why. I need to get around to stabilize those, sorry.

@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2022

Codecov Report

Base: 96.36% // Head: 92.62% // Decreases project coverage by -3.73% ⚠️

Coverage data is based on head (616b156) compared to base (eab6e2c).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4364      +/-   ##
==========================================
- Coverage   96.36%   92.62%   -3.74%     
==========================================
  Files          45       96      +51     
  Lines        2281     3799    +1518     
  Branches      640      967     +327     
==========================================
+ Hits         2198     3519    +1321     
- Misses         80      264     +184     
- Partials        3       16      +13     
Impacted Files Coverage Δ
src/devtools/Logo.tsx
src/react/QueryClientProvider.tsx
src/core/utils.ts
src/core/subscribable.ts
src/core/queriesObserver.ts
src/devtools/styledComponents.ts
src/react/useBaseQuery.ts
src/react/useIsFetching.ts
...rc/createWebStoragePersistor-experimental/index.ts
src/core/onlineManager.ts
... and 131 more

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Collaborator Author

@Newbie012 Newbie012 left a comment

Choose a reason for hiding this comment

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

I think it should be written as eslint or ESLint rather than EsLint

Copy link

@KubaJastrz KubaJastrz left a comment

Choose a reason for hiding this comment

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

If you could include my name somewhere in this, that would be great. Thank you! Nice work!

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 1, 2022

@KubaJastrz absolutely: 6ec2643

@TkDodo TkDodo merged commit 5d5c0f0 into TanStack:main Nov 1, 2022
@TkDodo
Copy link
Collaborator

TkDodo commented Nov 1, 2022

@all-contributors add @KubaJastrz for code

@allcontributors
Copy link
Contributor

@TkDodo

I've put up a pull request to add @KubaJastrz! 🎉

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 1, 2022

@all-contributors add @Newbie012 for code

@allcontributors
Copy link
Contributor

@TkDodo

I've put up a pull request to add @Newbie012! 🎉

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 1, 2022

release is here 🎉

https://github.com/TanStack/query/releases/tag/v4.14.0

Thank you for contributing this. @KubaJastrz if you have a twitter handle, let me know to give you credit in the announcement :)

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 1, 2022

@Newbie012 I'm still not so sure about the auto fixing. Consider:

const foo = 5
const bar = 10

useQuery({ queryKey: [{ hello: 'world' }], queryFn: () => foo + bar, enabled: false })

the key will be fixed to:

[{ hello: 'world' }, foo, bar]

but it could also be:

[{ hello: 'world' }, bar, foo]

or, more likely:

[{ hello: 'world', foo, bar }]

The thing is: we don't know. Maybe we should downgrade this to a suggestion rather than a fix?

@Newbie012
Copy link
Collaborator Author

I think you're correct. I'll open a new PR that will downgrade it to a suggestion rather than an auto-fix.

@Newbie012
Copy link
Collaborator Author

fixes in #4425

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants