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

finer-grained reactivity for nested objects #2254

Closed
dan335 opened this issue Jun 24, 2014 · 7 comments
Closed

finer-grained reactivity for nested objects #2254

dan335 opened this issue Jun 24, 2014 · 7 comments

Comments

@dan335
Copy link

@dan335 dan335 commented Jun 24, 2014

Template.hello.helpers({
user: function() {
return Meteor.users.findOne(Meteor.userId(), {fields: {"game.blah":1}})
}
})

If anything in Meteor.user().game changes then it invalidates this helper even though the fields specify that it only uses Meteor.user().game.blah. So if Meteor.user().var is updated then this helper will re-run.

https://github.com/dan335/fieldtest
http://fieldtest.meteor.com/
https://groups.google.com/forum/#!topic/meteor-talk/zg2cElH-Aqw

@glasser glasser added the Blaze label Jun 27, 2014
@n1mmy n1mmy removed the Blaze label Jun 27, 2014
@glasser
Copy link
Member

@glasser glasser commented Jun 28, 2014

Yeah, this is a bug in the minimongo implementation of observeChanges with fields filtering. A reduced sinppet that demonstrates it:

var X = new Package.minimongo.LocalCollection;
var id = "asdf";
X.insert({_id: id, foo: {bar: 123}});

X.find(id, {fields: {'foo.bar': 1}}).observeChanges({
  changed: function (id, fields) {
    throw Error("but nothing changed!");
  }
});
X.update(id, {$set: {'foo.baz': 456}});

The problem is that wrapCallback inside observeChanges does the filtering AFTER diff.js does a top-level-only diff. While it tries to skip empty diffs, it's actually impossible for it to do so at that level: without the preimage, it can't actually know if the fields it is selecting have changed or not.

Probably the projection needs to be applied before the diff rather than after. Might be tricky though because some parts of the code rely on the fact that the elements of query.results are direct references to the docs in the collection object...

Slava added a commit that referenced this issue Feb 24, 2015
…o's changed event.

This change actually introduces another layer of projections before diffing, in
addition to more projection after the diffing, on the result.

I didn't figure out why, but the tests fail if you remove either of them.

Adds tests. Fixes #2254.
Slava added a commit that referenced this issue Feb 27, 2015
…o's changed event.

This change actually introduces another layer of projections before diffing, in
addition to more projection after the diffing, on the result.

I didn't figure out why, but the tests fail if you remove either of them.

Adds tests. Fixes #2254.
glasser added a commit that referenced this issue Mar 6, 2015
This fails on both server and client.

This is also related to #2254.
@Slava Slava closed this in 16d5165 Mar 6, 2015
glasser added a commit that referenced this issue Mar 6, 2015
Projection functions act on *documents*, not the diffs that are passed
to changed callbacks.  For example, a projection `{'a.b': 1}` applied to
the *document* `{a: undefined}` will result in `{}`, but the *diff*
of `{a: undefined}` on a query with that projection should still result
in a `changed` callback with that diff being invoked.

Refactor various parts of minimongo to always have a projectionFn for
each observed query (perhaps a trivial one).

Pass the projection into _diffQueryChanges so that it can apply the
projection *before* taking the diff, in the two cases in Minimongo which
use _diffQueryChanges instead of direct application. (We could instead
make the query's results and resultsSnapshot be projected, but Minimongo
currently relies on the fact that these data structures alias the
documents stored in the collection.)

Stop applying the projection to the diffs in changed via
wrapCallback. (We could still use wrapCallback to apply the projection
for added/addedBefore calls, but it seems more consistent to use the
same mechanisms for added that we use for changed.

Fixes #3800.  Fixes #2254. Fixes #3571.
@glasser
Copy link
Member

@glasser glasser commented Mar 6, 2015

Turned out this was related to #3571. Fixed!

@stubailo
Copy link
Contributor

@stubailo stubailo commented Mar 6, 2015

This is awesome! I can't wait to start using this functionality to optimize my helpers.

@dan335
Copy link
Author

@dan335 dan335 commented Mar 6, 2015

Wow yes that is awesome! Can't wait for this next release.

@Slava
Copy link
Member

@Slava Slava commented Mar 6, 2015

Great work, should help to make Blaze predictable in performance.

@rclai
Copy link

@rclai rclai commented Mar 6, 2015

Whoa! Does this mean nested fields have fine-grained reactivity now??

@glasser
Copy link
Member

@glasser glasser commented Mar 6, 2015

Well, it depends what you mean by that. observeChanges changed messages still have the same API: changes are reported at the top level. But if your observed query has a fields filter like {'a.b': 1}, then changed will now be correctly emitted if and only if a.b has changed.

cscott added a commit to cjb/codex-blackboard that referenced this issue Jan 10, 2016
Meteor release notes:
http://info.meteor.com/blog/meteor-104-mongo-cordova-template-subscriptions
https://github.com/meteor/meteor/blob/release/METEOR@1.0.4.2/History.md

Important performance improvements included in this release:
* If the oplog observe driver gets too far behind in processing the oplog,
  skip entries and re-poll queries instead of trying to keep up.
  meteor/meteor#2668
* Optimize common cases faced by the "crossbar" data structure (used by
  oplog tailing and DDP method write tracking).
  meteor/meteor#3697
* The oplog observe driver recovers from failed attempts to apply the
  modifier from the oplog (eg, because of empty field names).
* Avoid unnecessary work while paused in minimongo.
* Fix bugs related to observing queries with field filters: changed
  callbacks should not trigger unless a field in the filter has
  changed, and changed callbacks need to trigger when a parent of an
  included field is unset.
  meteor/meteor#2254

There are also API improvements which can lead to more efficient/less
mistake-prone subscriptions and observe.  We'll migrate to these new
APIs in the next few commits.

Change-Id: I58e464750a03d9cfedfa7f07c9e25e8a6460fcde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants