Add integration test for side effects, ban top-level property access

[filipesilva]

PR Checklist

Please check if your PR fulfills the following requirements:

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

PR Type

What kind of change does this PR introduce?

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• Build related changes
• CI related changes
• Documentation content changes
• angular.io application / infrastructure changes
• Other... Please describe:

What is the new behavior?

This test checks if the side effects for loading Angular packages have changed using https://github.com/filipesilva/check-side-effects.

Running yarn test will check all ES modules listed in side-effects.json.

Running yarn update will update any changed side effects.

To add a new ES module to this test, add a new entry in side-effects.json.

Usually the ESM and FESM should have the same output, but retained objects that were renamed during the flattening step will leave behind a different name.

This PR also introduces a new lint rule that bans top-level property access on modules that are meant to be side-effect free, because they can inadvertently lead to side effects.

#30239 contains some more information and comments about top-level property access.

Does this PR introduce a breaking change?

• Yes
• No

Other information

[filipesilva]

This computed method name seems to be causing all the retained code here.

Without it, the result is just

import "rxjs";

import "rxjs/operators";

[alfaproject]

The iterator symbol should probably just be polyfilled like any other polyfill (from core-js or manually)

[filipesilva]

We should avoid defining toplevel variables and then using them in toplevel code.

[filipesilva]

I think what causes this to be retained is the typeof comparison.

[filipesilva]

Not really sure why this is retained. check-side-effects assumes pure getters is true right now. Maybe something with all the ||.

[IgorMinar]

odd

[mary-poppins]

You can preview d25b540 at https://pr29329-d25b540.ngbuilds.io/.

[mary-poppins]

You can preview 9612baa at https://pr29329-9612baa.ngbuilds.io/.

[filipesilva]

@alfaproject not quite actually. Since it's an IIFE, it is immediately executed on loading.

So const P = (() => Promise.resolve())(); will be evaluated to const P = Promise.resolve(); on load. This will happen just once.

It's mostly a way to tell the bundler that whatever is in the IIFE is actually a single block which will be evaluated whole, instead of being the sum of its parts being evaluated toplevel.

This makes a bit more sense knowing that the code will go through the Build Optimizer prefix function transformer. This happens on Angular CLI, but we are considering just moving it to a build-time transform on Angular libraries instead.

As a concrete example, currently we have const P = Promise.resolve(); which reads something like this to the bundler:

When loading this module, access the resolve property of the Promise variable, call it, then assign it to the constant variable P.

The known problematic bit here is the property access, which can have side effects. You can tell the bundler/optimizer that there are no side effects for property access, but that is a blanket statement for the all code in the bundle and not just this part. Either no property access has side effects in your whole bundle, or all of it can.

With the changes I'm proposing, and after running through Build Optimizer, it will instead be const P = /*@__PURE__*/ (() => Promise.resolve())(); which reads rather differently:

When loading this module, assign the result of this side-effect free function call to the constant variable P.

This matters because the optimizer now knows that it doesn't matter what's in the IIFE much, it only matters that it is free from side effects and thus the whole thing can go away if not used.

[trotyl]

but we are considering just moving it to a build-time transform on Angular libraries instead.

@filipesilva A real Angular project would have many dependencies other than @angular/* ones, it's totally unrealistic to ask all library authors to wrap top-level properties.

So that the app built size would always dramatically increased after this change, isn't it?

[filipesilva]

@trotyl there's a couple of different things to answer in your comment so I'll separate the bits:

it's totally unrealistic to ask all library authors to wrap top-level properties.

It is. But Build Optimizer doesn't do that either. So nothing changes.

What is changing in this PR is that we're adding tests that validate that Angular libraries that aren't meant to have side effect (signalled via this field) really have none.

Then what would change in angular/angular-cli#14187 (that had to be reverted, but I want to get in again) is that we don't automatically assume that all libraries have side-effect free property access, which is unrealistic and breaks code at runtime

Which is to say we shift the load from assuming everyone has side-effect free property access to actually making the Angular libraries free from side effects.

For instance, here's the same PR for rxjs: ReactiveX/rxjs#4769. This PR also removed side effects that weren't even removable by assuming property access was free from side effects.

So that the app built size would always dramatically increased after this change, isn't it?

I don't think so. In angular/angular-cli#14187 I tried removing the Angular CLI optimization flag that assumed that property access was free from side effects and in AIO the difference was 0.4% (2kb) of the total bundle size.

Granted AIO is primarily using Angular libraries. But when I did those tests I didn't test it with the changes in this PR. I just tested it by removing the CLI flag.

Which is to say that even all the toplevel property access in Angular itself today didn't amount to more than 2kb in AIO.

It did amount to a lot in this specific Ivy test. But in angular/angular-cli#14316 we looked at why and it turns out that it was because of two truly unintended side effects.

[IgorMinar]

lgtm, but needs a rebase + in a separate PR we should get the FW team to add all of the missing entry points (e.g. @angular/common/http is missing)

[IgorMinar]

@filipesilva

What is changing in this PR is that we're adding tests that validate that Angular libraries that aren't meant to have side effect (signalled via this field) really have none.

this is not quite true. what that field signals is that if there are any side-effects then they can be safely ignored because they are just local side effects that need to be preserved if the module containing them survives tree-shaking in webpack.

there is a difference between side effect and non-local local (or module-local / symbol-local) side effect. The hierarchy is as follows:

eval-time (top-level) side-effects > module-local eval-time side-effects > symbol-local eval-time side-effects > run-time side-effects > no-side effects.

there are probably a few more levels in between.. but roughly speaking, it's important to keep in mind that side-effects is not a "yes" or "no" question/answer.

[alxhub]

@filipesilva I manually rebased this.

[mary-poppins]

You can preview c916f81 at https://pr29329-c916f81.ngbuilds.io/.

[mary-poppins]

You can preview e55db3c at https://pr29329-e55db3c.ngbuilds.io/.

[IgorMinar]

@filipesilva why this size regression?

[filipesilva]

I think this regression is due to IIFEs being more code, especially when transpiled to ES5. Most of those IIFEs remain because their variable is actually used in a default CLI app.

For instance

// original
const CORE_TOKENS = {
    'ApplicationRef': ApplicationRef,
    'NgZone': NgZone,
};
// transpiled to es5
var CORE_TOKENS = {
    'ApplicationRef': ApplicationRef,
    'NgZone': NgZone,
};

versus

// modified with IIFE
const CORE_TOKENS = (() => ({
  'ApplicationRef': ApplicationRef,
  'NgZone': NgZone,
}))();
// transpiled to es5
var CORE_TOKENS = (function () { return ({
    'ApplicationRef': 'ApplicationRef',
    'NgZone': 'NgZone',
}); })();

[filipesilva]

Tried to validate this by making some before/after diffs with prettified output. They are pretty noisy because vars change name.

But you can see it here: filipesilva/compare-toplevel-prop-iife@3f116e9#diff-7a9076d6d94e62c13d641aa71f19ae8eL4109

Before:

    var Os = {
      ApplicationRef: ri,
      NgZone: Fo
    };

After

    var Ou = function () {
      return {
        ApplicationRef: ni,
        NgZone: Ho
      }
    }();

Another example in filipesilva/compare-toplevel-prop-iife@3f116e9#diff-7a9076d6d94e62c13d641aa71f19ae8eL1558

I eyeballed the whole diff and couldn't see anything else out of place. A couple of blocks moved because their var name changed but that was it.

I also found what the concrete before and after numbers are though. It is now 166,558 before and 166,741 after.

So I think what's really happening is that this PR isn't increasing the size by more than 1%. Rather, the payload limited is listed as 164,945, which allows for values between -1% (163,295) and +1%(‭166,594). But the current size was already almost breaking the +1% threshold, and the 183 Bytes just pushed it over the edge.

[IgorMinar]

g3 failures look like flakes. I've kicked off a rerun for the failed targets.

[IgorMinar]

g3 is happy: https://test.corp.google.com/ui#id=OCL:248239756:BASE:248259306:1557888153442:60052ec

[ngbot]

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "angular" is failing
     status "google3" is failing
     forbidden labels detected: PR action: review
     1 pending code review

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

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