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

fix(@angular-devkit/build-angular): downlevel class properties when targeting Safari <=v15 #24357

Merged
merged 1 commit into from
Dec 2, 2022

Conversation

devversion
Copy link
Member

@devversion devversion commented Dec 1, 2022

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:
#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

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

@devversion devversion changed the title fix(@angular-devkit/build-angular): downlevel class properties when targeting Safari <v15 fix(@angular-devkit/build-angular): downlevel class properties when targeting Safari <=v15 Dec 1, 2022
// 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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI: Discussed with @clydin that we should just apply for v14 too. The range can be that loose just to be safe. older versions will have the plugin enabled anyway.

// 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+=)/;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a sanity test. Obviously it's not validating using AST, nor validating Safari. Should be helpful regardless though.

@devversion devversion added action: review The PR is still awaiting reviews from at least one requested reviewer target: patch This PR is targeted for the next patch release area: @angular-devkit/build-angular labels Dec 1, 2022
@devversion devversion marked this pull request as ready for review December 1, 2022 14:11
Copy link
Member

@clydin clydin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Just two suggestions:

  • Can you add an additional test case for an explicit safari <=15 browserslist?
  • Also can you add a comment that the default test case (no explicit browserslist) will need to be remove once Safari 17 is released to prevent this from failing?

…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;
```

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 angular#24355.
@devversion devversion force-pushed the safari-v15-scope-fix branch 2 times, most recently from 153f119 to 86b19c5 Compare December 1, 2022 16:29
@devversion
Copy link
Member Author

@clydin Updated.

@devversion devversion added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Dec 1, 2022
@devversion devversion removed the request for review from alan-agius4 December 1, 2022 16:32
clydin added a commit to clydin/angular-cli that referenced this pull request Dec 1, 2022
…ri <= v15 for esbuild

To provide a workaround for a Safari bug involving static fields and variable scoping,
the esbuild-based browser application builder will now downlevel static fields if Safari
(desktop or iOS) v15.x or earlier is within the target browsers for an application.
This is an esbuild variant of the fix for the Webpack-based builder. For more details
regarding the issue, please see: angular#24357
@angular-robot angular-robot bot merged commit 25eaaa2 into angular:main Dec 2, 2022
clydin added a commit to clydin/angular-cli that referenced this pull request Dec 2, 2022
…i <= v15 for esbuild

To provide a workaround for a Safari bug involving class fields and variable scoping,
the esbuild-based browser application builder will now downlevel class fields if Safari
(desktop or iOS) v15.x or earlier is within the target browsers for an application.
This is an esbuild variant of the fix for the Webpack-based builder. For more details
regarding the issue, please see: angular#24357
angular-robot bot pushed a commit that referenced this pull request Dec 2, 2022
…i <= v15 for esbuild

To provide a workaround for a Safari bug involving class fields and variable scoping,
the esbuild-based browser application builder will now downlevel class fields if Safari
(desktop or iOS) v15.x or earlier is within the target browsers for an application.
This is an esbuild variant of the fix for the Webpack-based builder. For more details
regarding the issue, please see: #24357
angular-robot bot pushed a commit that referenced this pull request Dec 2, 2022
…i <= v15 for esbuild

To provide a workaround for a Safari bug involving class fields and variable scoping,
the esbuild-based browser application builder will now downlevel class fields if Safari
(desktop or iOS) v15.x or earlier is within the target browsers for an application.
This is an esbuild variant of the fix for the Webpack-based builder. For more details
regarding the issue, please see: #24357

(cherry picked from commit cf2f30a)
@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 Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker area: @angular-devkit/build-angular target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Angular apps not working in Safari v15 due to ReferenceError
3 participants