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

Make tracked queries the default #2922

Closed
TkDodo opened this issue Nov 11, 2021 · 5 comments · Fixed by #2987
Closed

Make tracked queries the default #2922

TkDodo opened this issue Nov 11, 2021 · 5 comments · Fixed by #2987
Assignees
Labels
Milestone

Comments

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 11, 2021

currently, notifyOnChangeProps defaults to undefined, but many people have already stated that the tracked version would be what they'd see as "expected"

also: remove notifyOnChangePropsExclusion

The inverse array doesn't really give you much given that you can just use tracked queries. Also, it "breaks" every time we add a new property that changes often.

For example, we have recently added a isRefeching computed flag to the queryResult, and since this is based on isFetching, people that have excluded isFetching would now get their re-renders again.

@TkDodo TkDodo added the v4 label Nov 11, 2021
@TkDodo TkDodo added this to the v4 milestone Nov 11, 2021
@babycourageous
Copy link
Contributor

I'll give this one a go!

@babycourageous
Copy link
Contributor

babycourageous commented Nov 18, 2021

Would you prefer these be together in 1 PR or 2 separate PRs (one for making "tracked" the default and another for removing notifyOnChangePropsExclusion)?

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 18, 2021

You can do both in one PR if you like :)

@babycourageous
Copy link
Contributor

For the default behavior for notifyOnChangeProps is the intention that useQuery by default would behave as though notifyOnChangeProps: "tracked" was defined

useQuery('key', queryFn) === useQuery('key', queryFn, {notifyOnChangeProps: "tracked"})

@TkDodo
Copy link
Collaborator Author

TkDodo commented Nov 18, 2021

exactly. we can set the default value for notifyOnChangeProps to tracked, but people should still have the option to opt out of it, likely by doing notifyOnChangeProps: undefined. If it's difficult to distinguish between the explicit value undefined being passed and the key not being there (which could be), maybe we could:

  • remove tracked completely and just say if it's not defined, it tracks
  • add `notifyOnChangeProps: 'all' to opt out of the smart tracking

in any case, we should keep the additional array notation so that users can define the keys they want to observe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants