From 25eaaa24b51af400262b97b4d4be2391ebd4a82d Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Thu, 1 Dec 2022 10:20:27 +0000 Subject: [PATCH] fix(@angular-devkit/build-angular): downlevel class properties when targeting 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: https://github.com/angular/angular-cli/issues/24355#issuecomment-1333477033 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; ``` note that in practice TypeScript with `useDefineForClassFields = false` will put non-static members into the constructor as normal assignments regardless- so there would be no change by the Babel plugin. Fixes #24355. --- .../src/babel/presets/application.ts | 33 ++++++++++- .../build_angular/src/babel/webpack-loader.ts | 13 +---- .../tests/misc/safari-15-class-properties.ts | 56 +++++++++++++++++++ 3 files changed, 90 insertions(+), 12 deletions(-) create mode 100644 tests/legacy-cli/e2e/tests/misc/safari-15-class-properties.ts diff --git a/packages/angular_devkit/build_angular/src/babel/presets/application.ts b/packages/angular_devkit/build_angular/src/babel/presets/application.ts index 366f07e48a10..13b184d6e37e 100644 --- a/packages/angular_devkit/build_angular/src/babel/presets/application.ts +++ b/packages/angular_devkit/build_angular/src/babel/presets/application.ts @@ -14,9 +14,26 @@ import type { makeLocalePlugin, } from '@angular/localize/tools'; import { strict as assert } from 'assert'; +import browserslist from 'browserslist'; import * as fs from 'fs'; import * as path from 'path'; +/** + * 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 is technically not supported via https://angular.io/guide/browser-support, + // but we apply the workaround if forcibly selected. + 'Safari <=15', + 'iOS <=15', + ]), +); + export type DiagnosticReporter = (type: 'error' | 'warning' | 'info', message: string) => void; /** @@ -45,7 +62,6 @@ export interface ApplicationPresetOptions { linkerPluginCreator: typeof import('@angular/compiler-cli/linker/babel').createEs2015LinkerPlugin; }; - forcePresetEnv?: boolean; forceAsyncTransformation?: boolean; instrumentCode?: { includedBasePath: string; @@ -171,13 +187,26 @@ export default function (api: unknown, options: ApplicationPresetOptions) { ); } - if (options.forcePresetEnv) { + // 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 features + // based on the supported browsers in browserslist. + if (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 (options.supportedBrowsers.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, + include: includePlugins, exclude: ['transform-typeof-symbol'], }, ]); diff --git a/packages/angular_devkit/build_angular/src/babel/webpack-loader.ts b/packages/angular_devkit/build_angular/src/babel/webpack-loader.ts index 78af1b28d480..a43c01b03f3d 100644 --- a/packages/angular_devkit/build_angular/src/babel/webpack-loader.ts +++ b/packages/angular_devkit/build_angular/src/babel/webpack-loader.ts @@ -79,7 +79,6 @@ export default custom(() => { const customOptions: ApplicationPresetOptions = { forceAsyncTransformation: false, - forcePresetEnv: false, angularLinker: undefined, i18n: undefined, instrumentCode: undefined, @@ -105,14 +104,6 @@ export default custom(() => { shouldProcess = true; } - // 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 - // based on the supported browsers in browserlist. - customOptions.forcePresetEnv = true; - } - // Application code (TS files) will only contain native async if target is ES2017+. // However, third-party libraries can regardless of the target option. // APF packages with code in [f]esm2015 directories is downlevelled to ES2015 and @@ -121,7 +112,9 @@ export default custom(() => { !/[\\/][_f]?esm2015[\\/]/.test(this.resourcePath) && source.includes('async'); shouldProcess ||= - customOptions.forceAsyncTransformation || customOptions.forcePresetEnv || false; + customOptions.forceAsyncTransformation || + customOptions.supportedBrowsers !== undefined || + false; // Analyze for i18n inlining if ( diff --git a/tests/legacy-cli/e2e/tests/misc/safari-15-class-properties.ts b/tests/legacy-cli/e2e/tests/misc/safari-15-class-properties.ts new file mode 100644 index 000000000000..a0dce7186cff --- /dev/null +++ b/tests/legacy-cli/e2e/tests/misc/safari-15-class-properties.ts @@ -0,0 +1,56 @@ +import { expectFileToExist, readFile, writeFile } from '../../utils/fs'; +import { ng } from '../../utils/process'; +import { updateJsonFile } from '../../utils/project'; + +const unexpectedStaticFieldErrorMessage = + 'Found unexpected static field. This indicates that the Safari <=v15 ' + + 'workaround for a scope variable tracking is not working. ' + + 'See: https://github.com/angular/angular-cli/pull/24357'; + +export default async function () { + await updateJsonFile('angular.json', (workspace) => { + const build = workspace.projects['test-project'].architect.build; + build.defaultConfiguration = undefined; + build.options = { + ...build.options, + optimization: false, + outputHashing: 'none', + }; + }); + + // Matches two types of static fields that indicate that the Safari bug + // may still occur. With the workaround this should not appear in bundles. + // - static { this.ecmp = bla } + // - static #_ = this.ecmp = bla + const staticIndicatorRegex = /static\s+(\{|#[_\d]+\s+=)/; + + await ng('build'); + await expectFileToExist('dist/test-project/main.js'); + const mainContent = await readFile('dist/test-project/main.js'); + + // TODO: This default cause can be removed in the future when Safari v15 + // is longer included in the default browserlist configuration of CLI apps. + if (staticIndicatorRegex.test(mainContent)) { + throw new Error(unexpectedStaticFieldErrorMessage); + } + + await writeFile('.browserslistrc', 'last 1 chrome version'); + + await ng('build'); + await expectFileToExist('dist/test-project/main.js'); + const mainContentChromeLatest = await readFile('dist/test-project/main.js'); + + if (!staticIndicatorRegex.test(mainContentChromeLatest)) { + throw new Error('Expected static fields to be used when Safari <=v15 is not targeted.'); + } + + await writeFile('.browserslistrc', 'Safari <=15'); + + await ng('build'); + await expectFileToExist('dist/test-project/main.js'); + const mainContentSafari15Explicit = await readFile('dist/test-project/main.js'); + + if (staticIndicatorRegex.test(mainContentSafari15Explicit)) { + throw new Error(unexpectedStaticFieldErrorMessage); + } +}