query icon indicating copy to clipboard operation
query copied to clipboard

refactor(reactBatchedUpdates): remove the side effect that sets batchNotifyFn in react

Open RodSarhan opened this issue 2 years ago • 3 comments

This PR attempts to fix #4685 of v5 milestones

  • set sideEffects to false in react-query/package.json
  • remove the side effect import from index.ts
  • delete reactBatchedUpdates.ts, reactBatchedUpdates.native.ts, setBatchUpdatesFn.ts files and references to them

RodSarhan avatar Dec 24 '22 05:12 RodSarhan

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

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

codesandbox-ci[bot] avatar Dec 24 '22 05:12 codesandbox-ci[bot]

Thanks. I think we'll need to wait for the PR that removes the react17 test suite because in react17, the batching does make a difference :)

TkDodo avatar Dec 24 '22 08:12 TkDodo

Thanks. I think we'll need to wait for the PR that removes the react17 test suite because in react17, the batching does make a difference :)

Awesome thanks! Yeah I was wondering if the solution should be backwards compatible, but looks like you decided to completely drop support for react 17

RodSarhan avatar Dec 24 '22 08:12 RodSarhan

Codecov Report

:exclamation: No coverage uploaded for pull request base (v5@22be6d5). Click here to learn what that means. Patch has no changes to coverable lines.

Additional details and impacted files
@@          Coverage Diff          @@
##             v5    #4699   +/-   ##
=====================================
  Coverage      ?   92.27%           
=====================================
  Files         ?       88           
  Lines         ?     3585           
  Branches      ?      941           
=====================================
  Hits          ?     3308           
  Misses        ?      262           
  Partials      ?       15           

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

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

codecov-commenter avatar Jan 05 '23 10:01 codecov-commenter

react17 removal PR has been merged. can you solve the conflicts here please?

TkDodo avatar Jan 10 '23 10:01 TkDodo

Also, please add some lines to the migration guide, especially for react-native, where this might be needed to set manually.

TkDodo avatar Jan 10 '23 10:01 TkDodo

The merge conflict should be resolved. As for the migration guide, do we have any confirmation that this is still needed in react-native?

RodSarhan avatar Jan 10 '23 14:01 RodSarhan

The merge conflict should be resolved. As for the migration guide, do we have any confirmation that this is still needed in react-native?

Not really, but it depends on which react-native "architecture" you're using. I think what we can write is something along the lines of:

If your Framework supports a custom batching function, you can let TanStack Query know about it by calling, for example:

import { unstable_batchedUpdates } from 'react-native'
import { notifyManager } from '@tanstack/react-query
notifyManager.setBatchNotifyFunction(unstable_batchedUpdates)

solid does this with a batch function now that is not unstable, we could also point to that:

https://github.com/TanStack/query/blob/b0d3c62070e8960ced79e6686c6c2f0154582a55/packages/solid-query/src/setBatchUpdatesFn.ts#L1-L4

TkDodo avatar Jan 10 '23 15:01 TkDodo

Added the following to the v5 migration guide, please let me know if it needs any changes https://github.com/TanStack/query/pull/4699/commits/e1dc8edfc58758ef07e4747ffe3302f84fb52cde

RodSarhan avatar Jan 11 '23 14:01 RodSarhan

perfect, thank you 🙏

TkDodo avatar Jan 11 '23 15:01 TkDodo