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

Function passed to notifyOnChangeProps cannot return undefined #7426

Open
winghouchan opened this issue May 13, 2024 · 2 comments · May be fixed by #7441
Open

Function passed to notifyOnChangeProps cannot return undefined #7426

winghouchan opened this issue May 13, 2024 · 2 comments · May be fixed by #7441
Labels

Comments

@winghouchan
Copy link

Describe the bug

Background

notifyOnChangeProps is a query option that determines if the query should notify listeners if query properties change. It has the following type:

export type NotifyOnChangeProps =
| Array<keyof InfiniteQueryObserverResult>
| 'all'
| (() => Array<keyof InfiniteQueryObserverResult> | 'all')

Depending on the value set, it has the following behaviours:

  • undefined: The query will track which properties are accessed and only notify listeners if those properties change. This is the default option.
  • string[]: The query will notify listeners if any of the properties in the array change.
  • 'all': The query will notify listeners if any properties change.

It can also be set with a function with the signature () => string[] | 'all'. The query will run the function and follow the behaviour listed above that matches the returned value.

With the available non-function options of undefined, string[], or 'all', it follows that the function should also be able to return undefined. However, the current type signature does not allow it.

Real world example

One real world example of the function passed to notifyOnChangeProps returning undefined comes from React Query's documentation itself, for disabling re-renders on out of focus screens on React Native. See this TypeScript Playground for a reproduction.

Impact

Allowing the function passed to notifyOnChangeProps to return undefined should not have any negative impact. At runtime, the query observer sets the value of notifyOnChangeProps or the value of the call to notifyOnChangeProps if it was a function to the same variable. This means if undefined is a valid value for notifyOnChangeProps then a call to notifyOnChangeProps should also be allowed to be undefined. See:

const notifyOnChangePropsValue =
typeof notifyOnChangeProps === 'function'
? notifyOnChangeProps()
: notifyOnChangeProps

Your minimal, reproducible example

https://www.typescriptlang.org/play/?#code/JYWwDg9gTgLgBAbzgVwM4FMCKz1QJ5wC+cAZlBCHAOQACMAhgHaoMDGA1gPRTr2swBaAI458VAFDi0WUXgAUCcXGVwRuPADFGALjj1UeRqzhyAlHAC8APjgwoOADRKVa-AGl0eXQG0qrvB54VAC6TipwjBAwwCR4APKMAMIAFkwA5ugACuRgqLpmljbIjAAm6CTAjOglToSmQA

Steps to reproduce

Set notifyOnChangeProps query option to a function that (could) return undefined.

Expected behavior

No type error should occur.

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

N/A

Tanstack Query adapter

None

TanStack Query version

5.29.2

TypeScript version

5.4.5

Additional context

No response

@winghouchan
Copy link
Author

The resolution should be to just update

| (() => Array<keyof InfiniteQueryObserverResult> | 'all')

to

| (() => Array<keyof InfiniteQueryObserverResult> | 'all' | undefined) 

Happy to open a PR if my understanding described above is correct.

@TkDodo
Copy link
Collaborator

TkDodo commented May 15, 2024

Yes you are right. It will already work at runtime because if undefined is returned from the function, we treat it the same as if the prop itself is undefined:

notifyOnChangePropsValue ?? this.#trackedProps,

Happy to accept a PR to adapt the types 👍

@TkDodo TkDodo added the types label May 15, 2024
winghouchan added a commit to winghouchan/react-query that referenced this issue May 16, 2024
…option to return `undefined`

`undefined` is a valid return value for the function value of `notifyOnChangeProps`. See linked issue for more details.

Fixes TanStack#7426.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants