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

@angular/elements + zone.js change detection related to RxJs stream runs in wrong zone #31870

Closed
tomastrajan opened this issue Jul 27, 2019 · 13 comments
Labels
area: elements Issues related to Angular Elements area: zones type: confusing
Milestone

Comments

@tomastrajan
Copy link
Contributor

tomastrajan commented Jul 27, 2019

Reproduction

You can get running example of the last solution using this one liner git clone https://github.com/tomastrajan/angular-elements-cd-example.git && cd angular-elements-cd-example && npm i && npm start This will be working, it can be broken by removing { ngZone: (window as any).ngZone } from elements main.ts file (as per description)

Description

Basically it's about using @angular/elements inside of the Angular application. It all started with weird behavior in change detection but ONLY in rxjs observable streams...

The stream in element did not trigger change detection...

Example, we have an @Input() username and we display it inside of element but also use it to load repositories from github API, the change to username will:

  1. trigger component re-render ( and display actual username in template )
  2. trigger ngOnChanges
  3. trigger backend request as a part of observable stream ( also receive data)

but the change will NOT:

  1. trigger re-render as a result of received data from backend

Causes

It seems to be the case when current zone is logged out inside of the rxjs steam it is the parent Angular app zone instead of the element zone...

Explored solution 1: zone.js/dist/zone-patch-rxjs

I tried to solve it in various ways, the most promising was to add import 'zone.js/dist/zone-patch-rxjs'; to the element BUT....

This leads to second more serious problem ....

If the consumer SPA or ( any of it libs ) already uses import 'zone.js/dist/zone-patch-rxjs'; then any subsequent call of import 'zone.js/dist/zone-patch-rxjs'; which calls Zone.__load_patch('rxjs', handler ...) will be ignored because in zone.js there is

static __load_patch(name, fn) {
            if (patches.hasOwnProperty(name)) {                    // only first rxjs patch will be applied
              if (checkDuplicate) {
                    throw Error('Already loaded patch: ' + name);   // this will not happen because checkDuplicate = false by default
                }
            }
            else if (!global['__Zone_disable_' + name]) {
                const perfName = 'Zone:' + name;
                mark(perfName);
                patches[name] = fn(global, Zone, _api);             // patch rxjs
                performanceMeasure(perfName, perfName);
            }
        }

so then if element comes with it's own rxjs in its bundle and tries to import 'zone.js/dist/zone-patch-rxjs'; patch it, it will be ignored because zone already has rxjs patch...

Explored solution 2: pass parent Angualr app NgZone into the element

In terms of solutions what we do now is

In element:

  1. do not import any zone or any patch
  2. use .bootstrapModule(AppModule, { ngZone: (window as any).ngZone })

In consumer SPA:
1.

export class AppModule {
  constructor(private ngZone: NgZone) {
    (window as any).ngZone = this.ngZone // store reference on window to be used by element during its bootstrap
  }
}

The solution seems pretty dirty, would be curious about why we can't patch more than one rxjs and in what direction will the element go and if anybody ever struggled with this ?

Reproduction (copy)

You can get running example of the last solution using this one liner git clone https://github.com/tomastrajan/angular-elements-cd-example.git && cd angular-elements-cd-example && npm i && npm start

The error behavior (when rxjs does NOT trigger change detection after receiving of data) we can simply remove { ngZone: (window as any).ngZone } from the element main.ts file

Questions

  1. why is rxjs broken at all ? (everything else seems to be working)
  2. why we're not allowed to zone-patch-rxjs for multiple rxjs bundles in the single page ?
  3. which solution makes most sense based on the roadmap of Angular and elements?
  4. what can go wrong when sharing parent ANgular NgZone instance with the child element?
  5. are the other solutions ( which does NOT involve manual CD because we need to convert existing Angular apps into elements without major rewrite)?

cc: @JiaLiPassion @robwormald @manfredsteyer

Versions

Angular / CLI / elements 8

@JiaLiPassion
Copy link
Contributor

@tomastrajan, I will check it. Thanks.

@ngbot ngbot bot added this to the needsTriage milestone Jul 27, 2019
@JiaLiPassion
Copy link
Contributor

@tomastrajan , this is really a very interesting issue, we may write a blog about it, I think I know the reason, but I am not sure I can explain it with text clearly, I will try to organize my wording first and talk to you soon. Also I will also try to find the solution, now I think share parent ngZone is the most easy one, but will also consider some other options.

@JiaLiPassion JiaLiPassion added the area: elements Issues related to Angular Elements label Jul 28, 2019
@tomastrajan
Copy link
Contributor Author

@JiaLiPassion thank you very much and looking forward to read the post!

@JiaLiPassion
Copy link
Contributor

@tomastrajan. So here is the reason.

  1. why not trigger re-render as a result of received data from backend, the 1st reason is because httpclient.get is an async operation (xhr). So any async operation can reproduce this issue, for example, the following code will also make the page not render.

guide-repos.component.html

<p>Username: {{username$ | async}}</p>

guide-repos.component.ts

    this.username$ = this.trigger$.asObservable().pipe(
      filter(Boolean),
      debounceTime(500),
      switchMap(username => {
        return of(username + 'test');
      })
    );
  1. the 2nd reason is the observable is triggered by this.trigger$ which is originally triggered by ngOnChanges from parent component( in this case, the app.component)

  2. so the internal reason is the app.component and gitrepos component they are in different ngZone.

Screen Shot 2019-07-28 at 22 37 31

So the trigger chain of the this.repos$ will look like

  1. app.component -> 2. set username prop to gitrepos (https://github.com/angular/angular/blob/master/packages/elements/src/component-factory-strategy.ts#L165-L168) -> 3. schedule change detection -> 4. run change detection with ngZoneOutside -> 5. call gitrepos.component.ngOnChange with ngZoneOutside -> 6. the xhr or setTimeout (debounceTime operator) will be scheduled with ngZoneOutside -> 7. when those async operation or operator callback is invoked, they will be invoked in ngZoneOutside not ngZoneInside.

  2. solution

  • so share parent ngZone can resolve this issue.
  • another solution is now zone.js already monkey-patch all customElements api such as attributeChangedCallback, so the call such as gitrepos.setAttribute() will run with ngZoneInside, but this case use the code (https://github.com/angular/angular/blob/master/packages/elements/src/component-factory-strategy.ts#L165-L168), so we may also need to patch the property setter here.
  • another approach is scoped zone, which is similar with the 2nd solution, to make sure everything happened inside angular elements use the ngZoneInside.

Will need more discuss of this, so currently the easiest walk around is to share the ngZone.

Thanks.

@tomastrajan
Copy link
Contributor Author

@JiaLiPassion thank you very much for such an in-depth answer! This makes a lot of sense and also corresponds with what we found when we logged out current zone inside of the stream operator.

We will proceed with the parent ngZone solution for now as .setAttribute() basically removes one of the best features of Angular which is the seamless template binding [username].

Hope we will not encounter any other issues 😅

Anyway, thank you again and please, don't forget to add a link to a eventual blog post if that happens, wish you a great day!

@tomastrajan
Copy link
Contributor Author

@JiaLiPassion I added description of the issue and possible solutions also here, maybe the project can be also of interest for you, it simplifies lazy loading of elements dramatically ;)

@JiaLiPassion
Copy link
Contributor

@tomastrajan, thank you very much for creating the documentation, and I will try to find out an elegant solution for this issue. Have a nice day 😄

@tomastrajan
Copy link
Contributor Author

Hey @JiaLiPassion ! One more thing, the solution with reusing parent NgZone seems not to be working when we use custom HTTP_INTERCEPTORS inside of the element, the rxjs stream is the executed in the <root> zone instead of the angular zone. Any ideas ?

@JiaLiPassion
Copy link
Contributor

@tomastrajan , could you create a reproduce sample of HTTP_INTERCEPTORS? Thanks.

@tomastrajan
Copy link
Contributor Author

Hey @JiaLiPassion , I debugged it further and it turned out to be something else, basically a stream was subscribed in runOutsideAngular and this made then any subsequent subscription to a other stream which was part of the first one to also run in <root> zone.

The solution was to NOT subscribe in runOutsideAngular so that the sub-stream is not pulled out and then everything works as expected.

@gkalpak
Copy link
Member

gkalpak commented Oct 27, 2020

AFAICT, this issue has been fixed with #37814 (included in v10.1.0). I am going to close this.
If you are still running into problems, feel free to open an new issue, @tomastrajan.

@gkalpak gkalpak closed this as completed Oct 27, 2020
@tomastrajan
Copy link
Contributor Author

@gkalpak thank you, I will try it out! 👍

@angular-automatic-lock-bot
Copy link

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: elements Issues related to Angular Elements area: zones type: confusing
Projects
None yet
Development

No branches or pull requests

3 participants