@angular/elements component events are called from an inconsistent zone

[divdavem]

I'm submitting a...

[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Performance issue
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question
[ ] Other... Please describe:

Current behavior

Event handlers in an angular element (registered as a custom element with @angular/elements), are called either from the angular zone or from the root zone depending on how the element is instantiated, leading to different behaviors:

• if the element is in the initial markup of the page (before the custom element is registered), then event handlers are called from the angular zone (as expected), and the change detection and refresh of the template are done correctly
• if the element is created from the angular zone (after the custom element is registered), then event handlers are called either from the angular zone (in Chrome, leading to a correct behavior) or from the root zone (in other browsers, leading to an incorrect behavior: the change detection is not done and the template is not refreshed)
• if the element is created from the root zone (after the custom element is registered), then event handlers are called from the root zone (both in Chrome and other browsers), leading to an incorrect behavior: the change detection is not done and the template is not refreshed.

Expected behavior

The angular element should behave correctly no matter how it is instantiated.
In the minimal reproduction sample, clicking on "Toggle display" should work no matter the line.

Minimal reproduction of the problem with instructions

https://stackblitz.com/edit/angular-dxu5td?file=src%2Fapp%2Fhello.component.ts
Depending on the line and the browser, clicking on the "Toggle display" works or has no effect.
In Chrome, only the first two lines work correctly. In other browsers, only the first line works correctly.

What is the motivation / use case for changing the behavior?

The current behavior is inconsistent. Event handlers should be called from the angular zone no matter how the component is instantiated, which would allow those components to be created from any other framework even if that framework does not run in the angular zone.

Environment

Angular version: 6.0.3

Browser:
- [x] Chrome (desktop) version 66.0.3359.181
- [x] Firefox version 60.0.1
- [x] IE version XX
- [x] Edge version XX

[remackgeek]

I have created a PR, #24185 for this (based on open PR 23885 from @JiaLiPassion

[divdavem]

@remackgeek Thank you for your quick response with a PR!

[robwormald]

In general, I'm not super excited about adding a bunch of affordances for Zones + Elements - the future of Elements is almost certainly Zoneless, so I'm concerned about adding a bunch of Zone specific APIs we have to support forever.

@remackgeek your PR seems fine, though it might be better implemented as an additional ElementStrategy/Factory (or potentially a subclass of the ComponentFactoryStrategy) - which developers who wanted to use Zones could pass into the createCustomElement call. The explicit pass in (vs detecting internally) means that if a developer doesn't pass the zone strategy in, it could be treeshaken away.

[JiaLiPassion]

@robwormald , I agree an additional ElementStrategy/Factory is better, by checking current NgZone is using zone.js or noopzone, we can use different Strategy.

And do you have chance to take a look at my proposal? https://github.com/JiaLiPassion/angular-elements-with-zone
I believe we can make zone.js only active inside Angular Elements without impact outside world, so user can have another option to develop Angular Elements.
The benefits of this idea are :

1. user can develop Angular Elements just like developing normal Angular Components.
2. user can easily migrate existing Angular Components to Angular Elements.

Please check it when you have time. Thank you!

[remackgeek]

@robwormald, At my company there is a big opportunity for using elements in older applications -- and the developers are used to change detection that Just Works(tm), so I think the zone use case brings a lot of value.

So you are thinking something like this to enable zones:

const strategyFactory = new ComponentNgElementZoneStrategyFactory(HelloComponent, injector);
const customElement = createCustomElement(HelloComponent, { injector, strategyFactory });

and then a strategy with methods something like this?

connect(element: HTMLElement) { this.insureAngularZone( () => { super.connect(element);});}
private insureAngularZone(fn: () => any) { NgZone.isInAngularZone() ? fn() : this.ngZone.run(fn); }

I'll update the PR

[remackgeek]

...or, add a new optional field to the configuration:
const insureAngularZone = true;
const customElement = createCustomElement(HelloComponent, { injector, insureAngularZone })

and change the factory constructor to add the boolean (defaulting to false). Then you wouldn't need to have a new factory and add it to the public API

Let me know which way you prefer.

[robwormald]

@remackgeek more like the first one - it doesn't require changing the API surface of createCustomElement

[otodockal]

I don't know guys, I've seen Angular elements as a great opportunity to promote more OnPush, fewer dependencies (Zone.js)...and the locality ;-)

[remackgeek]

@robwormald, I've updated the PR per your comments. Please take a look.

[LinboLen]

same issue occurred to me

[krawetko]

What is the plan on this? Having an opportunity to export existing AngularComponents and use them in non Angular or legacy AngularJS application with Zone/Change Detection working out of the box would be a huge use case for my company (and for many others with legacy / non angular systems i believe)

[remackgeek]

@gkalpak , if you are going to leave this for external library support, could you please take PR 26216, to avoid having to use knowledge of internal data structures?

[gkalpak]

Sounds reasonable. It seems that ComponentNgElementStrategy was meant to be public API all along.

Unfortunately, it seems that #26216 has slipped through the cracks and is quite old now (not to mention failing CI). Would someone be interested in submitting a new PR with this change. (It would be rad if someone wanted to add some API docs for NgElementStrategy too 🤩)

BTW, just to be clear, I did not suggest we should leave this for external library support (that is just one of the options and we might indeed end up deciding to go that way).

[angular-automatic-lock-bot]

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.}