autorun can cascade through zone.run()

[staeke]

Currently, calls like this.autorun(..., true) can cascade. By that I mean that the zone.run(...) call can synchronously lead to components being created, initialized and thus other autoruns to be run. But...this is depending on context and how/when the first autorun is run. The consequence if/when this happens is that Tracker is in a computation when the zone.run kicks off change detection. Further autorun calls will just add to the current computation, although all "user code" has already completed. I suggest a change to something like

if (autoBind) {
  hAutorun = Tracker.autorun(() => {
            func();
            Tracker.afterFlush(() => {
                this._zone.run(() => {});
            });
        });
}

Can make a PR if you agree

[barbatus]

Good point. Ideally though, it would be checking if it's already running in the zone.
So we have events like beforeTask, afterTask. I guess it can be checked.

[staeke]

Fair point, although I'm not sure about the performance benefit. But I guess we could do something like

this._zone.onTurnStart.subscribe(() => this._inZone = true)
this._zone.onTurnDone.subscribe(() => this._inZone = false)
...
if (autoBind) {
  hAutorun = Tracker.autorun(() => {
      func();
      if (!this._inZone) {
        Tracker.afterFlush(() => {
            this._zone.run(() => {});
        });
      }
  });
}

Want a PR or will you fix?

[staeke]

By the way...shouldn't autoBind = true be default?

[barbatus]

It may not work if autoBind = true by default now. Zone checking may be needed for all method before, then will see.

Did you test it? If zone is checked we don't need Tracker.afterFlush any more, I guess.
Otherwise PR'd be good.

[staeke]

What I interpret your comment as is to do:

func();
if (!this._inZone) {
  this._zone.run(() => {});
}

Well, the way I see it we still need Tracker.afterFlush. In zone or not, we don't want the extra zone.run(..) call to be run inside our autorun since we're not interested in tracking new dependencies within it, and new autoruns may happen as a result of zone.run(), so it's simply not 100% safe.

I tried my code with zone checking above. I was never in zone. I tried it again with the ApplicationRef's global zone. Then I'm in zone fairly often. And...we save quite many global ApplicationRef.tick and then change detection runs by not running that too often. But...we're also scheduling a lot of runs ourselves with a lot of consecutive Tracker.autoFlush So, in order to mitigate that, I'd rather propose something like

// Static
let pendingTrackerFlush = false;
...
Tracker.autorun(() => {
    func();
    if (!this._inZone && !pendingTrackerFlush) {
        pendingTrackerFlush = true;
        Tracker.afterFlush(() => {
            this._zone.run(() => {
                pendingTrackerFlush = false;
            });
        });
    }
});

Now, another problem is _inZone. As we can't rely on our local zone for telling if we need ApplicationRef.tick() kicking off on zone end, we must look at the ApplicationRef's zone the way I see it. We could have it injected, but...then we're introducing another constructor parameter for a base class, which I think is unfortunate (see angular/angular#6972).

A middle ground might be to have the ApplicationRef constructor parameter there, but have it optional, since it's only for performance optimization reasons.

[barbatus]

Ok, I am back to this issue. Sorry, other stuff took some time.
So, the part about having access to the app's NgZone in all components is pretty clear and was fixed recently.
But it still is not clear for me why cascading of Tracker.autorun might be an issue.
I think the following construction is fairly possible in Meteor, i.e.:

Tracker.autorun(() => {
  ....
  Tracker.autorun(() => {
     ....
     dep2.depend();
  })

 dep1.depend();
})

Could you elaborate what issue you are facing exactly with this?

[staeke]

Sure, that construction is possible and useful. My problem is with unintentional cascading as a side effect of running zone.run(). The problem is that zone.run(fn) actually means (synchronously)

1. Switch async contexts (zones) and run fn
2. If we were calling this from outside an ng zone (e.g. by coming from a pre angular websocket yielding changes to minimongo), we will have run our last microtask, have the angular zone stable, and thus run appRef.tick(), all change detection, which may lead to a lot of nasty things being done as a result that we DON'T want to keep inside an autorun.

What we want is leaving the autorun as soon as fn completes. One way of doing this is ensuring callbacks ALWAYS run outside angular, and later scheduling a zone run() rather than invoking one synchronously. But we also do have knowledge about Angular and want to run as few ticks as possible. Thus maybe something similar would be good?

// static
let pendingRuns = new Set<Zone>();
const rootZone = Zone.current;

// patched Tracker.autorun
autorun(cb) {
    capZone = Zone.current;
    return Tracker.autorun(() => { // Unpatched
        if (Zone.current === capZone) {
            // This minimizes a tick as we'll get one tick "for free" and don't have a run() call finishing
            cb();
        } else {
            pendingRuns.add(Zone.current);
            rootZone.run(cb);
            rootZone.scheduleMacroTask(() => {
                const s = pendingRuns;
                if (s.size) {
                    pendingRuns = new Set<Zone>();
                    s.forEach(z => z.run(() => {}));
                }
            })
        }
    })
}

[barbatus]

@staeke right it seems to have sense. but why not then just setTimeout(ngZone.run(fn))? setTimeout is anyways patched.

[staeke]

Well, in the case where you are already in the right zone you avoid one tick. If the other case (I guess that's the one you meant), my code basically throttles all tick requests down to one. Just calling setTimeout wouldn't. It's quite typical that a lot of autoruns fire because of one change. It would be unfortunate to get ~100 tick requests (we've seen such cases).

If, however you mean checking if a timeout has been scheduled and then not schedule one, yeah I guess that would work. But since multiple Angular applications may be running you need to store onr flag per angular zone.

[barbatus]

I am not sure that scheduleMacroTask can do that throttling you mentioned w/o setTimeout.
Also, since we have MeteorComponent.autorun I am going to patch it instead of Tracker.autorun itself.

[staeke]

Probably was unclear. No, scheduleMicroTask doesn't accomplish the throttling. It's running them in the root zone with a pending check. If that's what you meant, yes definitely you can accomplish the same with setTimeout.

As for the component approach vs. patching Meteor.autorun, I'd like to comment on that too (possibly reaching out of the topic of this thread, then please excuse me). I think we also need to patch

• Meteor.subscribe
• Meteor.apply
• Mongo.Cursor.observe
• Mongo.Cursor.observeChanges
• Tracker.afterFlush
• (possibly some more)

...since these methods may all results in callbacks being called as a result of server operations, and thus DDP/websocket callbacks outside any angular zone.

Since we need to patch all these methods, I think it makes more sense to patch the general API rather than to require a component base class. Component base classes limits quite actively where you could safely use Meteor. You may for instance want to do it in services. Also, I think it makes sense to make it really hard to do anything wrong. I see this work as making Meteor "compatible" with zone js, not Angular per se. There is still utility for a MeteorComponent when it comes to automatically stop autoruns and subscriptions though, so I'm not against providing a base component for providing that functionality.

Do you agree?

[barbatus]

Global patching is certainly not elegant solution, also I tend to think that user can take care of himself. So I would rather extend, for example, NgZone with a method say runAsync and notify about use cases where it has sense.
Also on MeteorComponent, I plan to create small patching decorators (and probably interfaces) for main Meteor methods in the near future, so no one will have to extend MeteorComponent if there is no need.

[staeke]

Thanks for your reply Alex,

Elegant or not, that's the path zone.js takes itself. I would agree with you in the general case, but for a library that has as its core idea to "patch the world" I would humbly disagree with you. Especially since people would run into problems if they didn't. And, I can think of no case where you wouldn't want to restore the zone. And the WebSocket functions are already patched. As a side note, please keep in mind that this is actually how things would work if there was a Meteor.startup() or similar function that we could call from an Angular component.

Anyhow, even if you don't want to patch, it seems a wrapper/façade would be a more reasonable approach (with the annotation for automatic disposal of handles), as then it can be used from any code (not just components). Also annotations and base classes limit their use to TypeScript - some people prefer just plain javascript.

[barbatus]

I agree patching Meteor methods makes sense but in general (i.e., relatively to global zone for the sake to have good stack traces). There was a package actually that does it on Atmosphere. But we want here something more specific, i.e., running in Angular2 zones per app and with throttling,
so I wasn't sure if it's okay (btw we have that autoBind param so far). But it seems to be not a big deal if we omit it.