@remackgeek , sure, I think a demo to describe this issue will be helpful. So your use case is you will create the Angular Elements dynamically with such as setTimeout?

@gkalpak, yes. Please see the latest update -- if you choose zone behavior, this wraps all the strategy methods.

@gkalpak, @robwormald thanks for the review. Updated.

I picked ComponentNgElementZoneStrategyFactory out. and it works.

@robwormald @remackgeek @gkalpak what are your plans for this change? My company has built many Angular Components and having the opportunity to export them as WebComponent to integrate in other non Angular applications (with Zones/Change Detection working out of the box) would be a huge use case and argument to move a lot of development focus into Angular Components.

Does this ensure that external callbacks attached to our @outputs run outside of the Angular zone? I'd think that's desirable for performance and encapsulation reasons.

The existing code in this PR would ensure that we re-enter the zone on @input, so it should be safe as well.

To clarify my output runOutsideAngular suggestion, it would look something like this:

initializeOutputs() {
  // Sets this.events
  super.initializeOutputs();

  this.events = this.events.pipe(runOutsideAngularZone(this.ngZone));
}

function runOutsideAngularZone(ngZone: NgZone) {
  return <T>(source: Observable<T>) => NgZone.isInAngularZone() ?
      new Observable<T>((observer) => source.subscribe({
           next: (value) => ngZone.runOutsideAngular(() => observer.next(value)),
           error: (err) => observer.error(err),
           complete: () => observer.complete()
         })) : 
      source;
}

@kseamon, wouldn't there be cases where you want the event to be in the zone? Say a third-party element being used in an Angular SPA.

Per @tnicola's comment, each child app would have it's own ngZone, so leaving it should be fine and would not cause the parent app to leave its own zone.

@remackgeek, yeah, I am working on a scoped zone in zone.js, so in theory, we can have separate zone per Angular Element. angular/zone.js#1126

So, to summarize the various threads to make sure I'm following everything:

1. To support the multiple ngZone case, the wrappers should always call ngZone.run(), because the NgZone.isInAngularZone() check doesn't distinguish between ngZones. This will add performance overhead to the case where the wrapper is already in the correct ngZone. PR 1126 should address this.

2. In all cases, the strategy should avoid leaking it's ngZone in callbacks, so the connect wrapper should capture Zone.current before running in the ngZone and run the callback events in that zone.

This should handle the 3 broad use cases:

1. The outer zone is the same ngZone as the element -- both the element and the host belong to the same angular application.
2. The outer zone is the root zone -- the element was created in a setTimeout() or similar.
3. The outer zone is another ngZone -- the host is an angular app consuming separately built angular elements.

@tnicola, I updated the PR to check the specific ngZone while waiting for PR 1126 land

@kseamon, I did some testing -- the event handlers for the outputs already run in the zone they were created in, so I'm not sure where there would be a performance gain changing zones in the Observable. But the mechanics of zones are black magic to me, so let me know if I missed something.

I created a PR in zone.js here, angular/zone.js#1133, it will patch all CustomElement v1 APIs, so this issue will not exist if we run customElements.define in ngZone.

@JiaLiPassion 's PR has landed in zone.js, so I'd appreciate people testing against that and letting us know about any changes

@robwormald, sure, I will test it!

I ran my examples and everything worked as expected. Thanks, @JiaLiPassion !

Will the next version of zone.js be released with Angular 7?

@remackgeek, thanks for the confirmation, I am not sure the release date, several PRs are still pending review.

@JiaLiPassion , I did some more testing and I did find an issue. Your PR doesn't seem to also fix #23841, while this one does. I have example code here. A live example is here

@remackgeek, thank you for the testing, yes, it will not work because we still need to update the customElement code here, https://github.com/angular/angular/blob/master/packages/elements/src/create-custom-element.ts#L187, to make sure the propertyChange also in ngZone.
Because zone.js not released, so we have to make this change after a new release of zone.js.

@gkalpak, I'd be happy to work on this!

I'd guess that the PR version is more performant and handles the same cases, but I'm not a zone wizard. I shamelessly lifted the basic pattern from @JiaLiPassion in PR23885..

It would be cool to put this in ComponentNgElementStrategy, but then don't you lose the ability to tree-shake out the zone stuff for zoneless users?

don't you lose the ability to tree-shake out the zone stuff for zoneless users

Not sure 🤔 Why would that be the case? What part would prevent the tree-shaking?
Oh, are you referring to the NgZone class?

I did some quick-and-dirty performance checking -- on my machine, the overhead for wrapping in ngZone.run() is about 20ms for 1 million calls. The overhead for comparing zones in the "already in zone" case is lost in the noise.
If you're not worried about tree-shaking ngZone, I'll put together a PR to modify ComponentNgElementStrategy.

I don't think NgZone would get tree-shaken anyway (due to how it is loaded), so I would go for it.

The new PR is here: #37814.

Thx, @remackgeek ✨
Closing this PR in favor of #37814.

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}