test(solid-query) Add test for transition #4224

jeski-bright · 2022-09-26T12:35:06Z

Add test for running a query during transition.

~~Change createResource usage to only indicate the suspended loading state. Always serve the underlying data from the query state. Do not trigger mutations and refetches as often.~~

~~This is a workaround for the following issue: solidjs/solid#1249.~~

codesandbox-ci · 2022-09-26T12:37:21Z

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 399f18e:

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

TkDodo · 2022-09-26T12:55:43Z

Hi, let's wait for @ardeora to review your change, but in the meantime:

please create a test case that fails with the current implementation on main, but doesn't fail with your fix.

thank you 🙏

jeski-bright · 2022-09-26T13:13:13Z

Hi @TkDodo.

Test case added, hope it helps!

ardeora · 2022-09-26T16:42:01Z

Thanks! Yeah this a very interesting/smart approach. I had looked at something similar before but the current implementation worked fine. Though, I'm curious as to which exact part of the current implementation is breaking transitions. I'm at work right now. But will take a deeper look at it in a few hours.

packages/solid-query/src/createBaseQuery.ts

ardeora · 2022-09-27T03:05:55Z

packages/solid-query/src/__tests__/transition.test.tsx

+        return true
+      })
+
+      return <div>{state.data ? 'Message' : 'never'}</div>


One thing that still breaks (even with the current implementation) is that if you use the Show component instead so

<Show when={state.data}> <div>Message</div> </Show>

still doesn't work 😭 There's maybe something goofy over here with solid js not resolving the reactivity correctly. I'm trying to create a minimal reproduction of where things are going wrong, but it'll take me more time

That's true :( But I believe this must be fixed in solid-js core.

This however works:

<div> <Show when={state.data}> <div>Message</div> </Show> </div>

It seems that the conditional component always needs to be wrapped in another one and Fragment doesn't help here either.
For this one I'd expect something wrong in the transformations made by X-plugin-solid packages.

This (along with the Show) issue seem to be fixed in solid-js already: solidjs/solid@ea2c976, though not released yet. Currently my fix breaks the <Show when={...} case 😂 , so how should we proceed with this?

I believe that the test case could still be useful.

Ah yes! Thanks for creating an issue with Ryan about this. I built the latest version and it seems like I can get it working fine (current implementation) with the new fixes Ryan implemented. I will let solid-js push a new release out to solve this upstream issue. However, the transitions tests would be really useful. I would suggest only adding tests in this pull request for now :D Let me know if you have any questions or concerns!

Our CI wont let us publish until all tests pass. So for now can we make it it.skip. I'll go back ad remove the skip when we have a new solid-js version out

Left only the test & updated the commit message.

ardeora · 2022-09-28T06:27:04Z

@TkDodo We are looking good with this PR. @jeski-bright added a new test for solid-js transitions. The upstream issue should be fixed in a few days and we can unskip the test then. Feel free to merge 😄

Or do you suggest we hold on to this PR until the upstream issue is fixed?

TkDodo · 2022-09-28T07:00:31Z

@ardeora since this PR only adds a skipped test, I would wait until the upstream issue is fixed and then merge the test when we actually run it.

jeski-bright · 2022-09-29T06:26:45Z

Since Ryan released those changes, updated solid to 1.5.7 and enabled the test. Please double check this, as during the first run of the test suite I got 1 failing test: useQuery › should not fetch when switching to a disabled query, subsequent re-runs were all green...

codecov-commenter · 2022-09-30T04:34:46Z

Codecov Report

Base: 96.36% // Head: 96.94% // Increases project coverage by +0.58% 🎉

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4224      +/-   ##
==========================================
+ Coverage   96.36%   96.94%   +0.58%     
==========================================
  Files          45       86      +41     
  Lines        2281     3273     +992     
  Branches      640      881     +241     
==========================================
+ Hits         2198     3173     +975     
- Misses         80       98      +18     
+ Partials        3        2       -1

Impacted Files	Coverage Δ
src/core/retryer.ts
src/react/useBaseQuery.ts
src/core/query.ts
src/devtools/Logo.tsx
src/react/useIsFetching.ts
src/core/utils.ts
src/core/onlineManager.ts
src/core/queriesObserver.ts
src/core/queryClient.ts
src/core/focusManager.ts
... and 121 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.

ardeora · 2022-09-30T04:36:28Z

@TkDodo The changes are looking good here and test:ci works fine on my local machine. But CI on github actions fails. Any idea what's going on here? 😄 I can see it complaining about not finding pnpm on disk

TkDodo · 2022-09-30T04:55:58Z

Yes I fixed this in my other PR. Sorry, I should port the fix directly to main. Will do later today.

ardeora · 2022-09-30T04:58:32Z

No worries! Thanks for the quick reply. Thanks @jeski-bright! We'll get this merged when CI is fixed

TkDodo · 2022-09-30T06:32:55Z

it's merged. please update with main to make the pipeline work :)

TkDodo

what is the empty file named change please? can we remove that?

jeski-bright · 2022-10-02T13:45:44Z

what is the empty file named change please? can we remove that?

Sorry for that, must've been checking something. Removed.

TkDodo requested a review from ardeora September 26, 2022 12:54

ardeora requested changes Sep 27, 2022

View reviewed changes

jeski-bright force-pushed the main branch from 252cd58 to e2e7e88 Compare September 27, 2022 05:41

jeski-bright mentioned this pull request Sep 27, 2022

Using transitions with suspense and @tanstack/solid-query breaks reactivity. solidjs/solid#1249

Closed

jeski-bright force-pushed the main branch from e2e7e88 to acda52e Compare September 28, 2022 05:33

jeski-bright changed the title ~~fix(solid-query) work around reactivity issue when using transitions~~ test(solid-query) Add test for transition Sep 28, 2022

Jedrzej Sadowski added 3 commits September 29, 2022 08:17

test(solid-query) Add test for transition

841680d

change!

eaf7c4b

chore(solid-js) update to most recent version and enable test

df4d07a

jeski-bright force-pushed the main branch from acda52e to df4d07a Compare September 29, 2022 06:23

Merge branch 'main' into main

66cd5b3

TkDodo reviewed Sep 30, 2022

View reviewed changes

Delete change

7a68494

Merge branch 'main' into main

399f18e

TkDodo approved these changes Oct 8, 2022

View reviewed changes

TkDodo merged commit f5fb92c into TanStack:main Oct 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test(solid-query) Add test for transition #4224

test(solid-query) Add test for transition #4224

jeski-bright commented Sep 26, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 26, 2022 •

edited

Loading

TkDodo commented Sep 26, 2022

jeski-bright commented Sep 26, 2022

ardeora commented Sep 26, 2022

ardeora Sep 27, 2022

jeski-bright Sep 27, 2022

jeski-bright Sep 27, 2022

ardeora Sep 27, 2022

ardeora Sep 27, 2022

jeski-bright Sep 28, 2022

ardeora commented Sep 28, 2022

TkDodo commented Sep 28, 2022

jeski-bright commented Sep 29, 2022

codecov-commenter commented Sep 30, 2022 •

edited

Loading

ardeora commented Sep 30, 2022

TkDodo commented Sep 30, 2022

ardeora commented Sep 30, 2022

TkDodo commented Sep 30, 2022

TkDodo left a comment

jeski-bright commented Oct 2, 2022

test(solid-query) Add test for transition #4224

test(solid-query) Add test for transition #4224

Conversation

jeski-bright commented Sep 26, 2022 • edited Loading

codesandbox-ci bot commented Sep 26, 2022 • edited Loading

TkDodo commented Sep 26, 2022

jeski-bright commented Sep 26, 2022

ardeora commented Sep 26, 2022

ardeora Sep 27, 2022

Choose a reason for hiding this comment

jeski-bright Sep 27, 2022

Choose a reason for hiding this comment

jeski-bright Sep 27, 2022

Choose a reason for hiding this comment

ardeora Sep 27, 2022

Choose a reason for hiding this comment

ardeora Sep 27, 2022

Choose a reason for hiding this comment

jeski-bright Sep 28, 2022

Choose a reason for hiding this comment

ardeora commented Sep 28, 2022

TkDodo commented Sep 28, 2022

jeski-bright commented Sep 29, 2022

codecov-commenter commented Sep 30, 2022 • edited Loading

Codecov Report

ardeora commented Sep 30, 2022

TkDodo commented Sep 30, 2022

ardeora commented Sep 30, 2022

TkDodo commented Sep 30, 2022

TkDodo left a comment

Choose a reason for hiding this comment

jeski-bright commented Oct 2, 2022

jeski-bright commented Sep 26, 2022 •

edited

Loading

codesandbox-ci bot commented Sep 26, 2022 •

edited

Loading

codecov-commenter commented Sep 30, 2022 •

edited

Loading