Skip to content

Commit

Permalink
fix(@angular-devkit/build-angular): downlevel class properties when t…
Browse files Browse the repository at this point in the history
…argeting Safari <=v15

The Angular compiler is dependent on static fields being attached to
user-defined classes. e.g. `static ecmp = defineComponent`.

These static fields sometimes rely on variables from outside of the
class. e.g. the Angular compiler generates constants for content
projection that are then accessed in the static field initializer.

Surprisingly such access to these variables may break in Safari <v15
when a page is loaded without devtools open. The bug (already solved in
v16 of Safari)- is very subtle, hard to re-reproduce but basically
variable scope tracking is broken. This bug is triggered by additional
parenthesis in the initializer expression. See:
https://bugs.webkit.org/show_bug.cgi?id=236843.

The TypeScript compiler may generate such additional parenthesis when
it tries to adjust the `this` context when invoking methods, such as for
defining animations in the `ecmp` definition.

More details can be found here:
angular#24355 (comment)

To ensure Angular applications are not subject to this bug when
targeting Safari <v15. v15 Safari, both for iOS and Mac is still part of
the default CLI browserslist with `last 2 Safari majors` (at time of
writing).

Note that it is important that the Babel plugin properly handles the
downleveling of static block-defined members. TypeScript will transform
static fields, like `static ecmp` into `static { this.ecmp = X }` when
`useDefineForClassFields = false` (which is the case for CLI apps). The
class properties plugin from Babel seems to handle this in an acceptable
way. Unlike actual static fields, Babel will not use helpers like
`defineProperty` for such extracted static blocks though. e.g.

See repro: https://gist.github.com/devversion/dec0dea26e348c509921bf62079b60be

```js
class Test {
  x = true;

  static b = true;
  static {
    this.a = true;
  }
}

// into

class X {
  constructor() {
    _defineProperty(this, "x", true);
  }
}
_defineProperty(X, "b", true);
X.a = true;
```

ds
  • Loading branch information
devversion committed Dec 1, 2022
1 parent 5ca7317 commit c381e2e
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,16 @@ import type {
import { strict as assert } from 'assert';
import * as fs from 'fs';
import * as path from 'path';
import browserslist from 'browserslist';

/**
* List of browsers which are affected by a WebKit bug where class field
* initializers might have incorrect variable scopes.
*
* See: https://github.com/angular/angular-cli/issues/24355#issuecomment-1333477033
* See: https://github.com/WebKit/WebKit/commit/e8788a34b3d5f5b4edd7ff6450b80936bff396f2
*/
const safariClassFieldScopeBugBrowsers = new Set(browserslist(['Safari <=15', 'iOS <=15']));

export type DiagnosticReporter = (type: 'error' | 'warning' | 'info', message: string) => void;

Expand Down Expand Up @@ -172,12 +182,23 @@ export default function (api: unknown, options: ApplicationPresetOptions) {
}

if (options.forcePresetEnv) {
const selectedBrowsers = options.supportedBrowsers;
const includePlugins: string[] = [];

// If a Safari browser affected by the class field scope bug is selected, we
// downlevel class properties by ensuring the class properties Babel plugin
// is always included- regardless of the preset-env targets.
if (selectedBrowsers && selectedBrowsers.some((b) => safariClassFieldScopeBugBrowsers.has(b))) {
includePlugins.push('@babel/plugin-proposal-class-properties');
}

presets.push([
require('@babel/preset-env').default,
{
bugfixes: true,
modules: false,
targets: options.supportedBrowsers,
targets: selectedBrowsers,
include: includePlugins,
exclude: ['transform-typeof-symbol'],
},
]);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ export default custom<ApplicationPresetOptions>(() => {
// Analyze for ES target processing
if (customOptions.supportedBrowsers?.length) {
// Applications code ES version can be controlled using TypeScript's `target` option.
// However, this doesn't effect libraries and hence we use preset-env to downlevel ES fetaures
// However, this doesn't effect libraries and hence we use preset-env to downlevel ES features
// based on the supported browsers in browserlist.
customOptions.forcePresetEnv = true;
}
Expand Down

0 comments on commit c381e2e

Please sign in to comment.