meteor / react-packages

Meteor packages for a great React developer experience
http://guide.meteor.com/react.html
Other
571 stars 157 forks source link

useTracker erroneously fires when using collection finds with sorts #318

Closed robwheatley closed 3 years ago

robwheatley commented 3 years ago

Hi there,

In a nutshell, if you have something like this in your tracker: Collection.find({}, {fields:{thing:1, anotherThing:1}}).fetch()

The the tracker will update only if 'thing' or 'anotherThing' changes. You can change anything else in a document and the tracker won't fire.

But if you have a sort too, like this: Collection.find({}, {sort:{thing:1}}, {fields:{thing:1, anotherThing:1}}).fetch()

The tracker will fire if ANYTHING changes in a document. It doesn't matter what you are sorting on, what fields you are fetching or if the sort has an affect on the outcome.

Background and more detail: I'm using useTracker to track changes in a Meteor collection. I'm only interested in certain changes, so in the .find() I have filters and fields applied. I expect the tracker to only fire if something that I'm filtering for in a field that I'm fetching changes. And that is true, BUT, I also have a sort applied to the find() and if you do that, then the tracker fires regardless of what changed in the collection.

It doesn't seem to matter what you are sorting on, just the presence of a sort tricks the tracker into thinking there was a change, even though the results are exactly the same..

Here's a very simplified example.

Imagine I have a Tasks collection. A task can belong to a project, and can be sorted by dueDate, so each task record has: _id title projectId dueDate completed (bool for whether the task was completed or not)

Let's pretend that for this tracker, I don't care if the task title updates and I don't care what order I get the tasks. I would set my tracker up like this:

  const {tasks} = useTracker(()=> {
      return Tasks.find({projectId:props.projectId},
                        {fields: {projectId:1, dueDate:1, completed:1}}).fetch()
    },[props.projectId]
  )

Now, if I change the title of a task in this project, the tracker won't fire. Perfect.

But, if we did care about the sort order and we wanted to sort the find by dueDate, we would change the tracker code to this:

  const {tasks} = useTracker(()=> {
      return Tasks.find({projectId:props.projectId},
                        {sort:{dueDate:1}}, 
                        {fields: {projectId:1, dueDate:1, completed:1}}).fetch()
    },[props.projectId]
  )

And this is where the problem comes in, now if you change the title of a task, then the tracker fires, even though the result is exactly the same.

Any thoughts?

CaptainN commented 3 years ago

That is actually a feature of Tracker. It's always updated whenever any document field changes, even if you filter them out in the query. (I'm actually surprised to hear it doesn't fire when sort isn't used - I would have thought it does.)

There have been requests for adding a comparator function (I actually added one recently, just have to write some tests) - but I'm hesitant to actually push it, because I'm concerned it's going to lead to a lot of bad decisions and increased support requirements. It would be a bad idea in most cases to use something like a generic deep-equal function, and I'm concerned folks will start to use patterns like that trying to outsmart Tracker and React.

The truth is, for most people, and for most uses, supplying no deps at all, and just allowing things to render extra times is completely fine. Deps and comparator functions should really be seen as optimization techniques that should only be used after extensive measuring. Especially a comparator.

Anyway, feel free to check out that branch I made for that.

robwheatley commented 3 years ago

OK, thanks for the info.

I got into this as I was on an 'optimization quest' (which sometimes leads to doing lots of messing about for very little gain!). Perhaps I should not worry so much.

But, given what you have said, I might do a little re-factoring. I have components higher up in the tree using a tracker looking for 'meta' info changes (order of items in a lists for instance) then lower in the component tree, trackers in the items looking for very specific changes on those items.

Given what you have said, I may as well not have trackers lower down in the tree and just pass info down the tree as props (given the update that the higher comp is going to do anyway). This is what I did in a previous version of the app anyway.

CaptainN commented 3 years ago

You could also use a Context provider, or something like redux.

Another pattern would be to put your subscription higher up in the tree, and only put the query for individual documents down lower. This is actually quite a smart pattern, and will become the suggested way to use useFind which we are working on in the new-hooks branch. That hook actually has a lot of optimizations similar to the ones you have shown.