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

Remove redundant optimisation of cache lookup for useGetMany(Aggregate) placeholderData #10256

Merged
merged 1 commit into from
Oct 31, 2024

Conversation

wattroll
Copy link
Contributor

@wattroll wattroll commented Oct 3, 2024

This is essentially a revert of commit e4e0f2c. See #7028 for why it was necessary. TanStack/query#6918 included equivalent optimisation (14aa69f), making it redundant here.

The dependency is bumped to be at least @tanstack/[email protected].

@wattroll
Copy link
Contributor Author

wattroll commented Oct 3, 2024

The typechecking errors appear to be caused by bumping react-query version and refer to code that haven't been changed here. I'll have a quick look at them, but should I rather:

a. fix them in this PR (separate commit), or
b. make another PR just fixing the type errors and have it merged first?

I am averse to removing optimisation without bumping the dependency bound since that would be an unnecessarily breaking change in a very subtle performance-impacting way.

@wattroll
Copy link
Contributor Author

wattroll commented Oct 3, 2024

I simply amended yarn.lock and specified the latest react-query version that doesn't trigger any type errors.

@wattroll
Copy link
Contributor Author

wattroll commented Oct 3, 2024

I also have confirmed that all 3 errors when building against latest react-query version appear in places where there is some degree of typechecker dodging. I assume that changes in react-query broke these coercions and they need to be refined or resolved soundly. Specifically:

  1. // @ts-ignore the types of the queryOptions for the getMAny and getList are not compatible
  2. ) as UseQueryResult<RecordType[], Error> & {
  3. ) as UseQueryResult<RecordType[], Error> & {

@wattroll
Copy link
Contributor Author

wattroll commented Oct 8, 2024

Rebased onto master in order to resolve the conflict in yarn.lock

@fzaninotto
Copy link
Member

Thanks!

Since you're bumping the required version for a dependency, this change should land in a minor version (e.g., 5.4) rather than a bug fix (e.g., 5.3.2). So please PR against the next branch instead of master.

This is essentially a revert of commit e4e0f2c. See marmelab#7028 for why it was
necessary. TanStack/query#6918 included equivalent optimisation [14aa69f],
making it redundant here.

The dependency is bumped to be at least [@tanstack/[email protected]].

[14aa69f]: TanStack/query@14aa69f
[@tanstack/[email protected]]: https://github.com/TanStack/query/releases/tag/v5.21.7
@wattroll
Copy link
Contributor Author

It is now rebased onto the next branch. There were no conflicts.

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

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

Thanks !

@slax57 slax57 added this to the 5.4.0 milestone Oct 31, 2024
@slax57 slax57 merged commit 010eb88 into marmelab:next Oct 31, 2024
14 checks passed
@slax57 slax57 changed the title Remove redundant optimisation of placeholder data query cache lookup Remove redundant optimisation of cache lookup for useGetMany(Aggregate) placeholderData Oct 31, 2024
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.

3 participants