Feature/use tracked query

[TkDodo]

I made a first POC of what we discussed here: #1554

I tried it out in the simple example and it worked fine - just looks too easy to be true. Am I missing something obvious? Apart from:

• tests (no idea how to test the proxy tracking, ideas welcome
• documentation

just wanted to ask upfront if a new hook is wanted / the way to go before I put any more effort into this :)

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tannerlinsley/react-query/mtoajecjf
✅ Preview: https://react-query-git-fork-tkdodo-feature-usetrackedquery.tannerlinsley.vercel.app

[boschni]

Nice! Couple of thoughts:

• Think we don't necessarily need a separate hook for this, maybe a query option to disable/enable this behavior like notifyOnChangeTracked: false.
• The proxy could be replaced with Object.defineProperty? In the descriptor you can define a get function which can be used to track if a property is accessed.

[TkDodo]

Think we don't necessarily need a separate hook for this, maybe a query option to disable/enable this behavior like notifyOnChangeTracked: false.

yeah, I’ve been thinking that a separate hook might be suboptimal, as you can’t combine it easily with useInfiniteQuery. The option might make more sense, and you can always set it globally to enable it everywhere. Let me see how that looks :)

The proxy could be replaced with Object.defineProperty?

Hmm i honestly don’t know. I thought proxies are the idiomatic way to do “interceptors”. Do you know of any example code with defineProperty I could take a look at?

[dai-shi]

I suppose you can use defineProperty to define object getters. It should work because there are only predefined set of properties.

For example:

consttarget={isFetching: true,data: {}}consttrackingObject={getisFething(){addNotifyOnChangeProps('isFetching')returntarget.isFetching},getdata(){addNotifyOnChangeProps('data')returntarget.data},}

defineProperty is a way do this dynamically.

[boschni]

Example with Object.defineProperty:

consttarget={isFetching: true,data: {}}constresult={}Object.keys(target).forEach(key=>{Object.defineProperty(result,key,{configurable: false,enumerable: true,get: ()=>{addNotifyOnChangeProps(key)returntarget[key]}});});

[TkDodo]

@boschni I changed the implementation from a new hook to a new option, and it also uses Object.defineProperty - works like before! Have a look please: 761a1a7

[TkDodo]

I did some documentation (API reference only), is that enough or do we want:

• a separate docs page?
• an example?

[TkDodo]

@boschni could you have another look please, i think i addressed all points 🙂

[boschni]

@boschni could you have another look please, i think i addressed all points 🙂

lgtm 👍

[TkDodo]

@boschni could you have another look please, i think i addressed all points 🙂

lgtm 👍

cool! @tannerlinsley if you're also happy I think we could merge this. Would you want more docs or examples?

[tannerlinsley]

🎉 This PR is included in version 3.6.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[faradaytrs]

@tannerlinsley why not having function to decide whether component should update? I would be similar to shouldComponentUpdate,
we could provide all the information to this function and depending on the result (true,false) rerender the component.

[TkDodo]

do you have a specific use case in mind where you would want to use such a function?

[faradaytrs]

do you have a specific use case in mind where you would want to use such a function?

I can't say for a now, but maybe for example if I have a request that often fails, I make few requests before I get one successful and I kinda don't need to rerender component until I got a successful result. I know this example is quite shallow, but I think such functionality is very useful anyway. It would pair well with select, sometimes you get the data you don't need to use and it's a good idea to decide whether to re-render the component. Of course, now you can, for example, wrap request function with another one that would throw an error on an undesirable result and configure query not to rerender on error, but the simple function would be much cleaner.

Now when you use notifyOnChangeProps: ['data'] you only can track that data changed, if you use the function I talked about above you can control HOW this data changes. It doesn't make a big difference but adds flexibility.

[TkDodo]

and I kinda don't need to rerender component until I got a successful result.

so if you use notifyOnChangeProps: 'tracked', and you only use data in your component, but not error or any loading flag, you will only re-render if data changes. If you start do observe errors as well, they will be "implicitly added" to notifyOnChangeProps. Unless there is reason that you are reading a value from the queryResult in you render function, but you don't want to render that component if that value changes, thus choosing to deliberately display outdated data, this should cover most bases.

what you cannot do at the moment is subscribing to parts of data with this. say you only access data.foo, and data.bar changes - your component will re-render with this approach.

that's where select comes in. with select, you can transform the data to what you need in your component, and only subscribe to those changes. So if you do: select: data => data.foo, you have just subscribed to foo only, and your component will no longer re-render if bar changes.

together, those two props (select and tracked queries) give you a very high amount of render optimization.

the problem with a "manual" function is the same as with shouldComponentUpdate. It is quite error prone to get right, leaving out things that you are actually using, but forget to add to that function will lead to stale data etc etc.

[faradaytrs]

and I kinda don't need to rerender component until I got a successful result.

so if you use notifyOnChangeProps: 'tracked', and you only use data in your component, but not error or any loading flag, you will only re-render if data changes. If you start do observe errors as well, they will be "implicitly added" to notifyOnChangeProps. Unless there is reason that you are reading a value from the queryResult in you render function, but you don't want to render that component if that value changes, thus choosing to deliberately display outdated data, this should cover most bases.

what you cannot do at the moment is subscribing to parts of data with this. say you only access data.foo, and data.bar changes - your component will re-render with this approach.

that's where select comes in. with select, you can transform the data to what you need in your component, and only subscribe to those changes. So if you do: select: data => data.foo, you have just subscribed to foo only, and your component will no longer re-render if bar changes.

together, those two props (select and tracked queries) give you a very high amount of render optimization.

the problem with a "manual" function is the same as with shouldComponentUpdate. It is quite error prone to get right, leaving out things that you are actually using, but forget to add to that function will lead to stale data etc etc.

Well using "tracked" with select has some obvious disadvantages. Sometimes you one want to track data.foo, but what you actually want to use is foo.bar. Select is nice for getting the necessary data from the response. If you make "tracked" rely on select, it makes select unuseful in some situations.
Another problem (maybe just for me). I don't really understand how "tracked" works. Data comes from JSON and I don't know how it detects changes. (Maybe it's problem that docs are not clear). I understand your position too, I will try to think of some situation where the current solution might cause inconvenience.

For me it's easy to think this way:

{
subscribe: ({data, isFetching, ...abc})=>true// check if data is worth rerendering,
select: (data)=>data.foo// prepare the data for the real use case}

[TkDodo]

I don't really understand how "tracked" works. Data comes from JSON and I don't know how it detects changes.

so tracked doesn't really do much 😅 . It only tracks access on the "top level" of the queryInfo to determine: Is the consumer interested in isLoading ? Or in data, or in error. It doesn't go "deep" into data.

where it plays nicely together with select is the "structural sharing" feature of react-query. If you have a json structure in data like this:

foo: { hello: 'world' },
bar: 1,

and the next response comes from the backend incrementing bar to 2, the structural sharing feature will make sure that data.foo stays referentially ident to the one from the "previous" render (I hope I'm not wrong on this 😅 ).

so if you only select: data => data.foo, your component will not re-render in this example if you set your subscriber to only be notified when data changes (either via setting nofiyOnChangeProps manually or by tracking and only using data in your component).

[faradaytrs]

so tracked doesn't really do much 😅 . It only tracks access on the "top level" of the queryInfo to determine: Is the consumer interested in isLoading ? Or in data, or in error. It doesn't go "deep" into data.

I actually got it, it's different from what I thought it is... I believe "tracked" should be the default option.
What I offered appears to be a different feature.

[TkDodo]

I believe "tracked" should be the default option.

yes, we discussed that in the PR, but for some edge cases (observing values in callback functions like event handlers), it was "too risky" for a minor release, so we made it opt-in. it might become default in the next major.

What I offered appears to be a different feature.

how about you open a new discussion with a feature request and we can discuss it further there :)

[faradaytrs]

I believe "tracked" should be the default option.

yes, we discussed that in the PR, but for some edge cases (observing values in callback functions like event handlers), it was "too risky" for a minor release, so we made it opt-in. it might become default in the next major.

What I offered appears to be a different feature.

how about you open a new discussion with a feature request and we can discuss it further there :)

well let me think of how it might look like and then i will.