Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(devtools): add types to Explorer #2949

Merged
merged 19 commits into from
Mar 15, 2022
Merged

refactor(devtools): add types to Explorer #2949

merged 19 commits into from
Mar 15, 2022

Conversation

Liam-Tait
Copy link
Contributor

Add types to Explorer component with as minimal functional changes as
possible while still getting type safety

#2742

@vercel
Copy link

vercel bot commented Nov 15, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tanstack/react-query/ET9df1xXf2KzYDgncuotnrWa3P7q
✅ Preview: https://react-query-git-fork-liam-tait-addexplorertypes-tanstack.vercel.app

Add types to Explorer component with as minimal functional changes as
possible while still getting type safety

2742
@codesandbox-ci
Copy link

codesandbox-ci bot commented Nov 15, 2021

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

Sandbox Source
tannerlinsley/react-query: basic Configuration
tannerlinsley/react-query: basic-typescript Configuration

@codecov
Copy link

codecov bot commented Nov 15, 2021

Codecov Report

Merging #2949 (e39f971) into master (c1ae82b) will decrease coverage by 0.00%.
The diff coverage is 87.09%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2949      +/-   ##
==========================================
- Coverage   96.40%   96.40%   -0.01%     
==========================================
  Files          45       45              
  Lines        2227     2278      +51     
  Branches      637      641       +4     
==========================================
+ Hits         2147     2196      +49     
- Misses         77       79       +2     
  Partials        3        3              
Impacted Files Coverage Δ
src/devtools/Explorer.tsx 76.92% <87.09%> (-0.55%) ⬇️
src/core/query.ts 100.00% <0.00%> (ø)
src/core/retryer.ts 100.00% <0.00%> (ø)
src/core/mutation.ts 100.00% <0.00%> (ø)
src/core/queryClient.ts 100.00% <0.00%> (ø)
src/react/useQueries.ts 100.00% <0.00%> (ø)
src/core/focusManager.ts 100.00% <0.00%> (ø)
src/react/useMutation.ts 100.00% <0.00%> (ø)
src/core/mutationCache.ts 100.00% <0.00%> (ø)
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c1ae82b...e39f971. Read the comment docs.

@Liam-Tait Liam-Tait marked this pull request as ready for review November 16, 2021 10:11
@TkDodo
Copy link
Collaborator

TkDodo commented Nov 17, 2021

can you try adding some more tests please

@Liam-Tait
Copy link
Contributor Author

can you try adding some more tests please

Can do

@Liam-Tait Liam-Tait marked this pull request as draft November 17, 2021 21:40
@Liam-Tait
Copy link
Contributor Author

@TkDodo are you be happy for me to do some additional clean up in this file too?

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 19, 2021

@TkDodo are you be happy for me to do some additional clean up in this file too?

sure :)

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 6, 2022

@Liam-Tait this looks like a nice addition, but the PR is still in draft state. Is ready for review? Thank you

@Liam-Tait
Copy link
Contributor Author

@TkDodo I think this still needed some more tests. I haven't added enough to maintain or increase coverage, however most of these changes are adding types to existing code.

@TkDodo TkDodo marked this pull request as ready for review February 8, 2022 20:28
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.

I'm fine with the test coverage. You've added types AND added a test. not sure how the coverage can go down but that doesn't matter :)

src/devtools/Explorer.tsx Outdated Show resolved Hide resolved
src/devtools/Explorer.tsx Outdated Show resolved Hide resolved
@Liam-Tait
Copy link
Contributor Author

@TkDodo Addressed issues, ready for a re-review

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 4, 2022

seems like there are some type errors now:

Error: src/devtools/tests/Explorer.test.tsx(39,11): error TS2322: Type '{ label: string; toggleExpanded: Mock<any, any>; pageSize: number; handleEntry: () => Element; expanded: false; subEntryPages: { label: string; }[][]; }' is not assignable to type 'IntrinsicAttributes & RendererProps'.
``

@Liam-Tait
Copy link
Contributor Author

@TkDodo Fixed

@TkDodo TkDodo merged commit c6594df into TanStack:master Mar 15, 2022
thomaspaulmann pushed a commit to thomaspaulmann/react-query that referenced this pull request Mar 18, 2022
* refactor(devtools): add types to Explorer

Add types to Explorer component with as minimal functional changes as
possible while still getting type safety

2742

* remove unused set param from explorer toggle

* Wrap Explorer toggle with useCallback

* Rename Explorer toggle to toggleExpanded

* Remove unused path

* Move subEntryPages definition next to usage

* Set type to be a string instead of string union

* Remove unused depth prop

* Move chunkArrays to own tested function

* set handleEntry as required

* Add LabelButton for accesibility

* fix test

* Remove shadowing

* Set subEntries as empty array by default

* Add type for property

* Convert handleEntry function to react component with entry props

* Use unknown for value

* Set RenderProps to required where possible

* Add required attributes to Explorer tests
@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 3.34.17 🎉

The release is available on:

Your semantic-release bot 📦🚀

@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 4.0.0-alpha.22 🎉

The release is available on:

Your semantic-release bot 📦🚀

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 25, 2022

@Liam-Tait I'm now seeing some react key related warnings when running the react-query devtools tests:

console.error
    Warning: Each child in a list should have a unique "key" prop.
    
    Check the render method of `Explorer`. See https://reactjs.org/link/warning-keys for more information.
        at HandleEntry (react-query/src/devtools/Explorer.tsx:242:21)
        at Explorer (react-query/src/devtools/Explorer.tsx:185:3)

can you maybe have a look at those please?

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 6, 2022

@Liam-Tait we sadly have 2 more (minor) issues reported that are likely introduced with this PR:


  1. Data Explorer now shows white background for expandable fields:

3.4.16:

Screenshot 2022-04-06 at 21 25 24

3.4.17:
Screenshot 2022-04-06 at 21 24 08

was this an intended change?


  1. expanded entries in the explorer now collapse if there is a background refetch that didn't actually change anything.

For example, take a look at this sandbox

  • click on the query in the devtools, and expand the owner field
  • then, re-focus the window

--> the owner collapses.

If you take the sandbox back to 3.4.16, owner stay expanded.


Could you take a look at these two things please? Should we open separate issues? Thank you!

@TkDodo
Copy link
Collaborator

TkDodo commented Apr 6, 2022

ah, issue 1 is caused by wrapping the clickable element in a button, but it's unstyled.

https://github.com/tannerlinsley/react-query/pull/2949/files#diff-a29eb5ef62b495218e2308396eb9064e59c6630c92421a6ceef78a0836d07fe3R17

@Liam-Tait
Copy link
Contributor Author

@TkDodo I'll have a look at these this weekend

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.

3 participants