fix(component): detect zone.js using instanceof comparison

[chrisguttandin]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://github.com/ngrx/platform/blob/master/CONTRIBUTING.md#commit
• Tests for the changes have been added (for bug fixes / features)
• Documentation has been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] Other... Please describe:

What is the current behavior?

Currently the detection of zone.js doesn't work when building a production build with the Angular CLI. The classname of the NoopNgZone class doesn't survive the minification.

What is the new behavior?

The new detection introduced in this PR uses the fact that NgZone calls apply() on a function that is passed to runOutsideAngular(). The NoopNgZone doesn't do that.

https://github.com/angular/angular/blob/master/packages/core/src/zone/ng_zone.ts#L226
https://github.com/angular/angular/blob/master/packages/core/src/zone/ng_zone.ts#L381

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

Another approach that I tried is to use runGuarded(). The NgZone will catch errors while the NoopNgZone doesn't do that. Therefore something like this will also work.

try {
    z.runGuarded(() => { throw new Error() });
} catch {
    return false;
}

return true;

However, when using zone.js the error will still be logged which is probably not wanted.

I'm not sure if this new test will trigger a change detection run when using zone.js. It might be beneficial to memoize the result. Please let me know if you want me to add that functionality.

[ngrxbot]

Preview docs changes for e8aa70a at https://previews.ngrx.io/pr2547-e8aa70a/

[brandonroberts]

@chrisguttandin will you rebase on master, and fix the merge conflicts?

[alex-okrushko]

public not needed, they are public by default

[alex-okrushko]

This feels overly complex.

Would the check for instanceof work? Or are none of those classes exported?

Also, Zone?.current?.name === 'angular' might work. Needs a bit of testing, since this is just guessing right now.

[alex-okrushko]

@chrisguttandin
Hey Christoph. So you actually ended up dragging me into this 😆

So this is a good enough check:

export function hasZone(z: NgZone): boolean {
  return z instanceof NgZone;
}

With a little caveat. User might provide their own Zone instance 😳 and then this would fail the check.

Alternatively this can be checked:

export function hasZone(z: NgZone): boolean {
  return !(z instanceof ɵNoopNgZone);
}

However this uses "private" API. 🤷‍♂️ So maybe the first one is good enough.

Here are the actual proper tests for hasZone, because otherwise we are just creating fake change-detector really, which I have zero confidence in - we weren't really checking if our function was able to identify it correctly.

describe('hasZone', () => {
  async function setup({ defaultZone }: { defaultZone: boolean }) {
    @Component({
      selector: 'body',
      template: '<div></div>',
    })
    class NgZoneTestComponent {
      checkNgZone = hasZone(this.ngZone);
      constructor(readonly ngZone: NgZone) {}
    }

    @NgModule({
      declarations: [NgZoneTestComponent],
      exports: [NgZoneTestComponent],
      bootstrap: [NgZoneTestComponent],
      imports: [NoopAnimationsModule],
    })
    class MyAppModule {}

    const platform = getTestBed().platform;
    const moduleRef = defaultZone
      ? await platform.bootstrapModule(MyAppModule)
      : await platform.bootstrapModule(MyAppModule, { ngZone: 'noop' });
    const appRef = moduleRef.injector.get(ApplicationRef);
    const testComp = appRef.components[0].instance;

    return { hasZone: testComp.checkNgZone };
  }

  it('returns false when default zone is used', async () => {
    expect(await setup({ defaultZone: true })).toEqual({ hasZone: true });
  });

  it('returns true when noop zone is chosen', async () => {
    expect(await setup({ defaultZone: false })).toEqual({ hasZone: false });
  });
});

[chrisguttandin]

Hi @brandonroberts, hi @alex-okrushko,

thanks for your feedback. I just rebased my changes on top of the master branch. That did reduce the number of changes made by this PR as some of the original changes are obsolete now.

@alex-okrushko I chose not to use ɵNoopNgZone since it is private, as you mentioned, and I thought it's better to not use it. However my solution is using an implementation detail which is probably equally bad. At least I'm convinced now that runOutsideAngular() does not trigger change detection since that is the use case it is designed for.

The only argument I have for checking runOutsideAngular() instead of using instance of NgZone is that the latter introduces a runtime dependency on NgZone even for apps which are not using it.

Anyway besides that I don't have a strong preference for either solution. Should I still make the changes you suggested, Alex? What about using an instanceof check with ɵNoopNgZone? It's not perfect but maybe the best solution out of the ones we have.

[alex-okrushko]

The only argument I have for checking runOutsideAngular() instead of using instance of NgZone is that the latter introduces a runtime dependency on NgZone even for apps which are not using it.

We are still pulling it in, so NgZone check is what I'd prefer ATM.

Thanks for working on it @chrisguttandin 👍

[chrisguttandin]

Now that I updated hasZone() and added tests for it, I realized that it isn't used anywhere anymore. 🤷‍♂️

Should I delete hasZone() and update isNgZone() instead?

[alex-okrushko]

Having "dead code" at this point of the library that should be fairly light-weight is surprising.

I'd suggest removing isNgZone instead.

hasZone is in the state that I'm comfortable with ATM - including being properly tested.

[BioPhoton]

Hi @AlexOkrushko I really like the test of ngZone over the component! 👍
This is definitely the best way I saw so far to test zone!

My 2 cent on importing zone to check it is i would also go for a method check. Imho the less dependencies on zone related stuff the better.

[alex-okrushko]

Angular is importing NgZone regardless, so it will be part of the bundle - if that's the concern.

[chrisguttandin]

I updated the code to replace any occurrence of isNgZone() with hasZone(). I also removed the mocked NgZone since it didn't pass the instanceof test and the tests seem to run with the real NgZone as well.

[BioPhoton]

Corner case:
Yes, I was thinking of that situation. I liked the if being zone independent.

But both is fine.