Skip to content

fix(zone): always run in zone for angular micro-frontends#2

Merged
remackgeek merged 6 commits intoremackgeek:masterfrom
tnicola:master
Aug 25, 2018
Merged

fix(zone): always run in zone for angular micro-frontends#2
remackgeek merged 6 commits intoremackgeek:masterfrom
tnicola:master

Conversation

@tnicola
Copy link
Copy Markdown
Contributor

@tnicola tnicola commented Aug 19, 2018

Hi @remackgeek ,

First of all, I would say thanks to you. You saved my life, great work!

I'm going to open this PR to fix all the cases in which we're importing elements created and defined in a external Angular micro-frondend.

Issue: Change Detection fails for Angular micro-frontends.

Maybe you're familiar with micro-frondents but for who don't know it's a technique in which a web app is a composition of features which are owned by independent teams (see https://micro-frontends.org/ for more details).

I tried to replicate a Micro-fronteds architecture in which, there is an Angular Host application and some other Angular components (my-custom-elements) created via @angular/elements. I'm importing the two elements from here: https://github.com/tnicola/my-custom-element but there's a problem, also with your library the change detection doesn't work for these external elements. This is due to the NgZone.isInAngularZone() check done at this line:

private runInZone(fn: () => any) { return NgZone.isInAngularZone() ? fn() : this.ngZone.run(fn); }
.

Since we have two 2 Angular apps, thehost and my-custom-elements, there will be also 2 NgZones and 2 ApplicationRefs, each app has it own. So, isInAngularZone() will return true for my-custom-elements even if we are in the Host NgZone (this is due because for each angular app a zone with 'angular' name is created and isAngularZone checks if there is at least one zone called 'angular'). What we need to get change detection working is to enter in my-custom-elements zone.

Removing the NgZone.isInAngularZone() check, as suggested in the PR won't break nothing in all the other cases and it would cost zero. If we already are in a zone, re-enter in the same zone is a noop operation.

Let me know if it's clear what I wrote. Thanks!

@remackgeek
Copy link
Copy Markdown
Owner

@tnicola , I'm glad someone else found this useful!
A couple of thoughts:

  • Wouldn't it be simpler to handle the multiple zone issue by pulling out zone.js of your elements and putting it in your page shell? Here's my example of that: angularJSMultipleElementsPOC, based on this article: How to build REAL Web Components with Angular 6.
  • I am not a zone expert by any means -- I based that "check the zone" pattern from code by people who do seem to know zone really well. If you think that change is something you need, you should bring it up in the discussion on this PR: fix(elements): initialization should run in ngZone (#24181), and see what the angular folks have to say. This library is based on that PR, which has the same pattern. I'm hoping that PR is the long-term solution.

@tnicola
Copy link
Copy Markdown
Contributor Author

tnicola commented Aug 20, 2018

@remackgeek Thank you for your comments.

@remackgeek
Copy link
Copy Markdown
Owner

@tnicola, I'm happy to take the change, but I want to keep my simple shell example. If you want to change your PR down to just the code change, I'll do the merge. If you don't want to bother with that, I can just make the change myself. Either way it will probably be a few days until I can get to it.

@remackgeek
Copy link
Copy Markdown
Owner

Also, you might be interested in this angular bug which is mostly the same issue: Hosting a Web Component inside an angular app. Data binding and change tracking is not working. #24901

@tnicola
Copy link
Copy Markdown
Contributor Author

tnicola commented Aug 22, 2018

@remackgeek Thanks! the demo was just to show you the issue and the resolution. I removed the example, the PR is updated.

@remackgeek remackgeek merged commit 8e76361 into remackgeek:master Aug 25, 2018
@remackgeek
Copy link
Copy Markdown
Owner

@tnicola , I merged your change and published version 6.0.4. I hope it works for you.

@tnicola
Copy link
Copy Markdown
Contributor Author

tnicola commented Aug 26, 2018

@remackgeek It works great! 🎉 Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants