Why are todoItems re-rendering when `list.incompleteCount` changes? about todos

[avital]

Hey @tmeasday, sorry for the late reply.

Can you point me to the code where the problem is happening? Like the original post on meteor-core where the user described which helper is being re-run multiple times?

[tmeasday]

Hey @avital, do you mean how do you know if the problem is happening?

If you add a console.log() to the checkedClass() helper in todos-item.js:14, you'll get a console each time a todo is rendered.

The repro is simply:

1. Open a list (notice N logs)
2. Check an item (notice another N logs).

If you open the version of todos in https://github.com/meteor/meteor, and do the equivalent logging, you'll see:

1. Open a list (notice N logs)
2. Check an item (notice 1 log).

I was going to look at this today too if I have time so if you make any headway/have any ideas, please post them here.

[tmeasday]

Like the original post on meteor-core where the user described which helper is being re-run multiple times?

I did link to it above, here it is again: https://groups.google.com/forum/#!searchin/meteor-core/Template$20helpers$20re-evaluate$20for$20each$20item/meteor-core/TZpUvWjaLO4/KN2xklayTlAJ

[avital]

Thanks, I meant where in the code I need to log to see the behavior (the checkedClass() helper you referred to)

I'll look into it

[avital]

I found this fact:

In lists-show.js, when changing:

  todos(listId) {
    return Lists.findOne(listId).todos();
  },

to

  todos(listId) {
    return Todos.find({listId: listId});
  },

the problem goes away.

I guess that's because Lists.findOne(listId) registers a dependency on the entire list document, which includes incompleteCount.

[...]

Yes.

You can solve this by changing that line to return Lists.findOne(listId, {fields: {_id: 1}}).todos();

[stubailo]

I remember at some point I tried to get around this problem by converting all properties to getter functions and querying Minimongo separately for every single field on the object, but I think it created too much overhead. Presumably GraphQL would solve the query aspect of this if we could implement fine-grained data tracking.

[tmeasday]

Thanks @avital, that's clearly it. Appreciate it.

[tmeasday]

Actually I'm not seeing that behaviour @avital.

I need to actually change the todoArgs helper to look like:

  todoArgs(todo) {
    const instance = Tracker.nonreactive(() => Template.instance());
    return {
      todo,
      editing: instance.state.equals('editingTodo', todo._id),
      onEdit(doEdit) {
        instance.state.set('editingTodo', doEdit ? todo._id : false);
      }
    };
  }

In order to see the "correct" behaviour.

The issue is that that second helper adds a depedency on Template.instance(), which creates a dependency on the instance's .data (which is the full list in this case).

@stubailo this is a problem actually!!! It means our habit of aggressively calling Template.instance() in Blaze helpers means that we easily can't do performance optimizations like this within the template.

Well actually we can, we just have to wrap Template.instance() in a Tracker.nonreactive(). But it's starting to get pretty obtuse. I dunno, maybe it's fine.

[tmeasday]

Also, for what it's worth, this issue pretty neatly highlights most of the issues I have with Tracker.

1. As an experienced Blaze/Tracker user, I still had a lot of trouble figuring out why the helper was getting re-called. [1]
2. It's in no way obvious (via documentation or any mechanism to trace it) that Template.instance() adds a dependency on the instance's data. Maybe you could intuit it or something, but there needs to be some way to definitively find out.
3. We don't have good tools (apart from the sledgehammer of Tracker.nonreactive) to change the reactivity properties of a data source like Template.instance(). There are some community tools for that though (like computed-field)

[1] I know a few tricks like using a console.trace() inside a Tracker.onInvalidate(), but due to the source of the invalidation being a generic "Blaze changed the data context of something (who knows what?)", I basically had to binary search debug by commenting stuff out.

[stubailo]

@tmeasday soooo do we need to recommend a pattern for calling Template.instance() non-reactively? Wow, it really sucks that Template.instance().data exists at all - it's 100% a duplicate of Template.currentData().

[tmeasday]

I think we don't need to recommend a pattern -- in general if you are calling Template.instance() in a helper, you are just setting the same dependency you already have with this, right?

It's just special cases like this, where you are trying to be tricky to control reactivity where it's good to know this. I think this example actually makes for an interesting little section of the Blaze guide to explain some of this.

[stubailo]

in general if you are calling Template.instance() in a helper, you are just setting the same dependency you already have with this, right?

That's not correct - this is the current data context, Template.instance() is the data context of the template inclusion. Could be different.

[tmeasday]

Yeah, that's what I mean by "in general" -- like it's unusual for it not to be.

If you are using {{#each in}} and {{#let}}, (and not {{#each}} and {{#with}}) then it'd always be the same, no?

[stubailo]

Yes, that's true. The extension of that project was going to be eliminating the data context entirely and passing template args through lexical scope, in which case the template wouldn't actually re-render because of a change in arguments - it would just get the new variable values. So I believe that would fix a lot of stuff here.

[tmeasday]

See #56