TanStack / query

πŸ€– Powerful asynchronous state management, server-state utilities and data fetching for the web. TS/JS, React Query, Solid Query, Svelte Query and Vue Query.
https://tanstack.com/query
MIT License
40.31k stars 2.7k forks source link

feat(devtools): Add framework agnostic devtools draft #5347

Closed ardeora closed 1 year ago

ardeora commented 1 year ago

This is a initial draft of the framework agnostic Tanstack Query devtools. Will add description as it gets more fleshed out.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

1 Ignored Deployment | Name | Status | Preview | Comments | Updated (UTC) | | :--- | :----- | :------ | :------- | :------ | | **query** | ⬜️ Ignored ([Inspect](https://vercel.com/tanstack/query/5AsmjhrfnZgCkpJqJMYm1anfWtR4)) | | | May 2, 2023 9:02am |
nx-cloud[bot] commented 1 year ago

☁️ Nx Cloud Report

CI is running/has finished running commands for commit bedc0db5293906a848ae79eb1fea3cd7aba4535b. 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 1 target - [`nx affected --targets=test:eslint,test:types,test:build,test:lib --base=a9d7a371aa3b65694ccf8bfc4ebc151832023097`](https://cloud.nx.app/runs/sFuK28T6IO)

Sent with πŸ’Œ from NxCloud.

codesandbox-ci[bot] commented 1 year ago

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 bedc0db5293906a848ae79eb1fea3cd7aba4535b:

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
ardeora commented 1 year ago

@TkDodo I think the build is working now! The demo of the new devtools can be seen here

https://codesandbox.io/p/sandbox/tanstack-query-example-react-basic-typescript-forked-m45rfk

Not sure why one of the Nx Cloud Orchestrator action is failing right now πŸ˜•

Props currently present:

export interface DevtoolsOptions {
  /**
   * Set this true if you want the dev tools to default to being open
   */
  initialIsOpen?: boolean;
  /**
   * The position of the React Query logo to open and close the devtools panel.
   * "top-left" | "top-right" | "bottom-left" | "bottom-right"
   * Defaults to 'bottom-left'.
   */
  buttonPosition?: DevtoolsButtonPosition;
  /**
   * The position of the React Query devtools panel.
   * "top" | "bottom" | "left" | "right" 
   * Defaults to 'bottom'.
   */
  position?: DevtoolsPosition;
  /**
   * Custom instance of QueryClient
   */
  client?: QueryClient;
  /**
   * Use this so you can define custom errors that can be shown in the devtools.
   */
  errorTypes?: DevToolsErrorType[];
}

Also The react devtools miss the following props:

For panelProps, closeButtonProps, toggleButtonProps and containerElement I wanted to ask how useful are these? Should we also keep these props in the newer devtools

Regarding styleNonce, I'll add this but would require a little more refactoring!

I will add tests for the new components and package too. I wanted to ask what should be the threshold for completeness on the remaining items so that we can get them to merge into the alpha branch? I can prioritise the items that are needed before they can be merged for the initial release πŸ˜„

Thanks, Hope you have a wonderful weekend!

TkDodo commented 1 year ago

Not sure why one of the Nx Cloud Orchestrator action is failing right now πŸ˜•

It's a general issue on alpha. Working on it...

TkDodo commented 1 year ago

Amazing ❀️. I'd have a couple of small feedback points:

1) I found it a bit weird that these icons get a pointer cursor, but aren't really clickable: Screenshot 2023-04-30 at 13 35 27

also, I cannot keyboard focus those buttons, so the tooltips are only shown when using the mouse.

2) I only knew how to hide the devtools (clicking the TanStack React Query v5 text) because I know how it works πŸ˜… . I don't think that's intuitive, and I could see first-time users struggling to minimize them again.

3) We fixed some time ago that the query status (stale in the image) doesn't stretch if the QueryKey is long. It seems like we're re-introducing that issue:

before:

Screenshot 2023-04-30 at 13 42 20

after:

Screenshot 2023-04-30 at 13 41 40

TkDodo commented 1 year ago

@ardeora upgrade to node18 fixed the issue, please merge alpha into here :)

ardeora commented 1 year ago

Amazing ❀️. I'd have a couple of small feedback points:

1) I found it a bit weird that these icons get a pointer cursor, but aren't really clickable:

Screenshot 2023-04-30 at 13 35 27

also, I cannot keyboard focus those buttons, so the tooltips are only shown when using the mouse.

2) I only knew how to hide the devtools (clicking the TanStack React Query v5 text) because I know how it works πŸ˜… . I don't think that's intuitive, and I could see first-time users struggling to minimize them again.

3) We fixed some time ago that the query status (stale in the image) doesn't stretch if the QueryKey is long. It seems like we're re-introducing that issue:

before:

Screenshot 2023-04-30 at 13 42 20

after:

Screenshot 2023-04-30 at 13 41 40

Thank you for all the great feedback!

  1. Ah yes! I'll make the tooltips more accessible! Apologies for missing that. Also the smaller icons are only available on smaller panel width sizes. Is there another way we should display the query status badges on smaller screens?

  2. I see there is a close button at the bottom left corner in the old devtools. Will adding that to the new one help?

  3. Ah I thought that was purposeful! I found it way more readable when it used to stretch the length of the query key. Also when it stretches the whole length I feel it gives a stronger visual feedback when a query's status has been changes. I can switch it to just take up the min height it requires. Let me know if you have a strong preference against it πŸ˜…

image

Thanks again for taking a look!!

TkDodo commented 1 year ago

the smaller icons are only available on smaller panel width sizes. Is there another way we should display the query status badges on smaller screens?

Oh I missed that. No, I think that's a perfect mix πŸ‘

I see there is a close button at the bottom left corner in the old devtools. Will adding that to the new one help?

yeah maybe top-right would actually be more where people expect it? Like a normal :x: to close it?

I can switch it to just take up the min height it requires. Let me know if you have a strong preference against it πŸ˜…

What I think would be best is if we didn't stretch out the QueryKey like that at all, because any QueryKey with a couple of entries will take up a lot of space vertically, but horizontally, we're leaving a lot of space unused

ardeora commented 1 year ago

If the queryKey is made to stretch out horizontally too, the serialized key will lose the prettified display. Would that make it more readable though?

TkDodo commented 1 year ago

ah, I see, we do this on purpose πŸ€” . I don't care too much tbh πŸ˜…

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: -0.67 :warning:

Comparison is base (c561e8b) 90.08% compared to head (5145a4c) 89.42%.

:mega: 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 ```diff @@ Coverage Diff @@ ## alpha #5347 +/- ## ========================================== - Coverage 90.08% 89.42% -0.67% ========================================== Files 76 71 -5 Lines 2672 2354 -318 Branches 737 608 -129 ========================================== - Hits 2407 2105 -302 + Misses 233 206 -27 - Partials 32 43 +11 ``` [see 9 files with indirect coverage changes](https://codecov.io/gh/TanStack/query/pull/5347/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=TanStack)

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

ardeora commented 1 year ago

Okay so I added the close button. The preview for it is here!

https://user-images.githubusercontent.com/45807386/235390119-31db1503-3d7f-478c-808f-1e16268089bb.mov

ardeora commented 1 year ago

And the tooltip is now keyboard focusable! And only shows cursor:pointer when we're showing the condensed view. Please let me know if there are any improvements that I can make. Thanks again for reviewing this

https://user-images.githubusercontent.com/45807386/235390233-ed336605-a966-415f-87d1-a513e43e6cd0.mov

TkDodo commented 1 year ago

that collapse button is amazing πŸš€

TkDodo commented 1 year ago

If we plan to release @tanstack/query-devtools as a separate npm package, we would need to add it to scripts/config.ts. Order is important there, so probably before react-query-devtools

ardeora commented 1 year ago

Okay moved the query-core to the peerDependencies looks like everything is still working! Also updated scripts/config.ts to release query-devtools too!

ardeora commented 1 year ago

Weird that it didn't create the codesandbox examples though πŸ˜• Not sure why that happened?

TkDodo commented 1 year ago

Weird that it didn't create the codesandbox examples though πŸ˜• Not sure why that happened?

could be an upstream issue with CSB

ardeora commented 1 year ago

Okay the codesandbox examples are back! πŸ₯³ The only that is complaining is the Nx Cloud Orchestrator again with a very cryptic error message πŸ˜…

TkDodo commented 1 year ago

if you scroll further up, you'll see a test failure:

 ❯ src/__tests__/useQuery.test.tsx  (127 tests | 1 failed) 8448ms
   ❯ src/__tests__/useQuery.test.tsx > useQuery > should update query state and not refetch when resetting a disabled query with resetQueries
     β†’ expected 3 to be 4 // Object.is equality

we do 3 retries to avoid flaky tests. I doubt that this one is broken by this PR, so maybe it's really flaky :/

TkDodo commented 1 year ago

you can see a better output in the nx dashboard: https://cloud.nx.app/runs/hGnuEGn28R

ardeora commented 1 year ago

Okay sweet! It finally worked! πŸ₯³

TkDodo commented 1 year ago

nice, do you want to ship it?

ardeora commented 1 year ago

We can! Since it's the alpha branch, It would be nice to get more feedback! Also I can add tests in a separate PR because I still need to figure how to run tests where one package's test suit (react query devtools) depends on another package (query-devtools) being built. Let me know if you have any other concerns, otherwise let's ship it! πŸ₯³

TkDodo commented 1 year ago

I still need to figure how to run tests where one package's test suit (react query devtools) depends on another package (query-devtools) being built.

hmm why would you need the package being built? react-query depends on query-core, so we just use the sources from that ...

but yeah, lets ship it and announce on twitter

ardeora commented 1 year ago

Yes! Works for query-core since it's pure JS. I'm still using JSX within solid-js and solids jsx and react's jsx compile very differently! Would be very different for svelte and vue too. There might be a way for it to make it work but I might need to spend a little more time!

rtrembecky commented 1 year ago

hi, I just tried upgrading @tanstack/react-query-devtools to 5.0.0-alpha.27 in a next.js project and I'm getting this error when running the dev app:

SyntaxError: Named export 'TanstackQueryDevtools' not found. The requested module '@tanstack/query-devtools' is a CommonJS module, which may not support all module.exports as named exports.

does this PR solve it? thanks

rtrembecky commented 1 year ago

confirming 5.0.0-alpha.30 solves it πŸ‘

by the way, it still brought an issue of

ReferenceError: document is not defined
    at template (file:///Users/my_user/my_project/node_modules/@tanstack/query-devtools/build/esm/index.mjs:809:13)
    at file:///Users/my_user/my_project/node_modules/@tanstack/query-devtools/build/esm/index.mjs:5042:31
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:526:24)
    at async importModuleDynamicallyWrapper (node:internal/vm/module:438:15) {

but that I identified as an error of next.js trying to import the component on the server, which doesn't have document. I went around it using dynamic import:

import dynamic from 'next/dynamic'

const ReactQueryDevtools = dynamic(
  () => import('@tanstack/react-query-devtools').then(({ReactQueryDevtools}) => ReactQueryDevtools),
  {
    ssr: false,
  },
)

I'll raise this as an issue, as you may want to keep it working on fullstack frameworks out-of-the-box, just as before

ardeora commented 1 year ago

confirming 5.0.0-alpha.30 solves it πŸ‘

by the way, it still brought an issue of


ReferenceError: document is not defined

    at template (file:///Users/my_user/my_project/node_modules/@tanstack/query-devtools/build/esm/index.mjs:809:13)

    at file:///Users/my_user/my_project/node_modules/@tanstack/query-devtools/build/esm/index.mjs:5042:31

    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)

    at async Promise.all (index 0)

    at async ESMLoader.import (node:internal/modules/esm/loader:526:24)

    at async importModuleDynamicallyWrapper (node:internal/vm/module:438:15) {

but that I identified as an error of next.js trying to import the component on the server, which doesn't have document. I went around it using dynamic import:


import dynamic from 'next/dynamic'

const ReactQueryDevtools = dynamic(

  () => import('@tanstack/react-query-devtools').then(({ReactQueryDevtools}) => ReactQueryDevtools),

  {

    ssr: false,

  },

)

I'll raise this as an issue, as you may want to keep it working on fullstack frameworks out-of-the-box, just as before

Hello! Apologies for the late reply! Thanks for trying it and yeah using next/dynamic is the correct approach to get the devtools working currently! I have a personal branch where I have gotten it to work out of the box like the previous devtools. I need some more testing before we merge it into the alpha branch. It would be helpful if you could create a new issue so that we can track the progress there! Hope you have a nice day!

rtrembecky commented 1 year ago

no problem, thanks for any reply πŸ˜„ issue already created here: #5369