Feature/use tracked query #1578
Conversation
add useTrackedQuery hook, which tracks notifyOnChangeProps automatically via a Proxy
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/tannerlinsley/react-query/mtoajecjf |
|
Nice! Couple of thoughts:
|
yeah, I’ve been thinking that a separate hook might be suboptimal, as you can’t combine it easily with
Hmm i honestly don’t know. I thought proxies are the idiomatic way to do “interceptors”. Do you know of any example code with |
|
I suppose you can use For example: const target = { isFetching: true, data: {} }
const trackingObject = {
get isFething() {
addNotifyOnChangeProps('isFetching')
return target.isFetching
},
get data() {
addNotifyOnChangeProps('data')
return target.data
},
}
|
|
Example with const target = { isFetching: true, data: {} }
const result = {}
Object.keys(target).forEach(key => {
Object.defineProperty(result, key, {
configurable: false,
enumerable: true,
get: () => {
addNotifyOnChangeProps(key)
return target[key]
}
});
}); |
different approach to tracking used props by using an option rather than a new hook, so that we can also easily combine this with useInfiniteQuery; also, using Object.defineProperty rather than a Proxy
add some documentation for notifyOnChangeTracked
|
I did some documentation (API reference only), is that enough or do we want:
|
add a test for notifyOnChangeTracked
keep trackedProps separate from notifyOnChangeProps and only merge them in shouldNotifyListeners
store tracked result object on queryObserver instance so that the reference stays the same between renders
create a new trackedResult whenever we create a new result this ensures that we keep referential identity of the tracked result object if nothing changes, but still get a new instance if we need to (to avoid stale closure problems)
add test for referential stability between re-renders; even if we are tracking changes, we should return the same object if we re-render (for some other reason than react-query changes) and nothing has changed
remove auto-resetting of tracked props, which means that every prop ever tracked will lead to a re-render; this is the "safe" side - we'd rather have an unnecessary re-render rather than a bug because we are not re-rendering when something is observed only in an effect
always re-render if we are tracking props, but not using any. In that case, trackedProps will be an empty array, which would lead to no re-renders at all (same as setting notifyOnChangeProps to empty Array)
conditionally create trackedResult - we don't need it if we are not actually tracking props
add a test for combining notifyOnChangeProps: 'tracked' and notifyOnChangePropsExclusion
update docs
|
@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? |
|
🎉 This PR is included in version 3.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
@tannerlinsley why not having function to decide whether component should update? I would be similar to shouldComponentUpdate, |
|
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. |
so if you use what you cannot do at the moment is subscribing to parts of data with this. say you only access that's where 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 |
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. 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
} |
so tracked doesn't really do much 😅 . It only tracks access on the "top level" of the where it plays nicely together with select is the "structural sharing" feature of react-query. If you have a json structure in and the next response comes from the backend incrementing so if you only |
I actually got it, it's different from what I thought it is... 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.
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. |
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:
just wanted to ask upfront if a new hook is wanted / the way to go before I put any more effort into this :)