fix(zone.js): defineProperty patch should not swallow error by JiaLiPassion · Pull Request #37582 · angular/angular

JiaLiPassion · 2020-06-15T03:52:55Z

zone.js monkey patches the Object.defineProperty API long time ago
angular/zone.js@383b479
to resolve issues in very old version of Chrome web which override the
property of CustomElements, and this is not an issue any longer, so
we may remove this monkey patch, since it may swallow the errors when the user
want to define property on unconfigurable or frozen object properties.

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 current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

Yes
No

Since zone.js doesn't patch Object.defineProperty and not swallow the error when trying to define property on unconfigurable or undefined object, some application currently not throw error will begin to break.

For example, in our saucelab test for IE and old firefox/chrome, before this PR, the code/spec are compiled to commonjs, and the js code will look like this.

Object.defineProperty(exports, "__esModule", { value: true })
...

And the exports is undefined, It should throw error that trying to defineProperty on undefined, since zone.js monkey patch the Object.defineProperty and will catch the error and do nothing, so the test cases will continue to work.
After this PR, zone.js will not do that any more, so the test cases will break.

Other information

devversion

Thanks @JiaLiPassion. It would be great if we can land this PR! It's quite confusing that zone.js currently has the capacity of hiding Object.defineProperty errors that could surface in production.

modules/tsconfig-umd.json

mhevery · 2020-07-10T19:54:19Z

presubmit

atscott · 2020-07-13T21:48:40Z

FYI I'm removing the merge label and adding back the presubmit label, as it seems there is one test that is consistently failing and doesn't appear to be a flake. @mhevery - Please add back the merge label once you've finished investigating.

mhevery · 2020-07-15T17:35:21Z

There is a CustomElement tests in g3 running on ie11-win7 which fails to bootstrap with this change. Seems like it can't be landed in the current form?

Perhaps we can conditionally add the defineProperty only if we detect that it is needed?

JiaLiPassion · 2020-07-16T00:47:28Z

@mhevery , got it, I will make defineProperty an optional module, and is that possible the g3 ie11-win7 test cases be updated? I mean after the defineProperty become an optional module, the current g3 ie11-win7 test need to load this module explicitly.

JiaLiPassion · 2020-07-24T23:36:35Z

@mhevery, @atscott , I just added a new commit to make define-property patch a new package called zone-patch-define-property, by default, it will not be loaded, to enable it, import it after loading zone.js

import 'zone.js/dist/zone';
import 'zone.js/dist/zone-patch-define-property';

AndrewKushnir · 2020-07-28T02:07:13Z

FYI, I've started global presubmit to check behavior in Google's codebase.

AndrewKushnir · 2020-09-01T00:32:39Z

Thanks for additional updates @JiaLiPassion. I've started a new presubmit and will let you know the results once I have them.

devversion · 2020-09-01T10:28:41Z

We can probably do some cleanup in tests, but this change might actually break more apps in some subtle ways, so I was wondering if we can come up with some strategy on how to handle this (externally as well as internally in Google's codebase). We should also consider if changing this behavior now worth introducing a breaking change.

It's a good question whether this is worth introducing a breaking change. Personally, I'd definitely say yes. Currently ZoneJS modifies native browser behavior. This wouldn't be a big deal at first glance, but it becomes a quite impacting issue IMO if the legacy browser ZoneJS bundle is conditionally loaded (as done in the CLI IIRC). It can then happen that your tests always pass, but once the application runs in production with a non-legacy browser, the application could suddenly break with runtime errors.

devversion

LGTM. Sounds reasonable to make an exception for the registerElement patch.

AndrewKushnir · 2020-09-01T17:43:28Z

FYI, I've started a new presubmit (+ global presubmit) and will keep this thread updated.

AndrewKushnir · 2020-09-02T23:50:55Z

Hi @JiaLiPassion, I'm still in a process of running tests in Google's codebase, but everything looks good so far. Since this is a breaking change, could you please update commit message to include the BREAKING CHANGES section (for ex. as it's done in this commit: 9d59b7d)?

I've re-requested review from @mhevery once again, since implementation has changed since his last review. Misko, could you please have a look again?

Thank you.

AndrewKushnir · 2020-09-03T00:59:56Z

FYI, global presubmit went well, @JiaLiPassion plz add the "action: merge" label once commit message is updated and there are no additional changes planned on your side. Thank you.

JiaLiPassion · 2020-09-03T01:30:19Z

@AndrewKushnir , thank you so much for the great help! I just updated the commit message to include the BREAKING CHANGE section.

packages/zone.js/lib/browser/define-property.ts

Close angular#37432 zone.js monkey patches the `Object.defineProperty` API long time ago angular/zone.js@383b479 to resolve issues in very old version of Chrome web which override the property of `CustomElements`, and this is not an issue any longer, so we want to remove this monkey patch, since it may swallow the errors when the user want to define property on unconfigurable or frozen object properties. But currently there are several apps and tests depends on this patch, since it also change the `configurable` property to `true` by default, so in this PR we update the logic to not to swallow error any longer unless the property is the callbacks of `document.registerElements`. BREAKING CHANGE: ZoneJS no longer swallows errors produced by `Object.defineProperty` calls. Prior to this change, ZoneJS monkey patched `Object.defineProperty` and if there is an error (such as the property is not configurable or not writable) the patched logic swallowed it and only console.log was produced. This behavior used to hide real errors, so the logic is now updated to trigger original errors (if any). One exception where the patch remains in place is `document.registerElement` (to allow smooth transition for code/polyfills that rely on old behavior in legacy browsers). If your code relies on the old behavior (where errors were not thrown before), you may need to update the logic to handle the errors that are no longer masked by ZoneJS patch.

Since the `defineProperty` not swallow error any longer, now the tests compile source code in `commonjs` mode, and the code generated includes the code like this ``` Object.defineProperty(exports, "__esModule", {value: true}); ``` And the `exports` is undefined in some browsers, but the error is swallowed before this PR, and all tests run successfully, but it is not correct behavior. After this PR, the code above failed. So we need to compile the source code in `umd` mode.

Since the `defineProperty` not swallow error any longer, now the tests compile source code in `commonjs` mode, and the code generated includes the code like this ``` Object.defineProperty(exports, "__esModule", {value: true}); ``` And the `exports` is undefined in some browsers, but the error is swallowed before this PR, and all tests run successfully, but it is not correct behavior. After this PR, the code above failed. So we need to compile the source code in `umd` mode. PR Close #37582

angular-automatic-lock-bot · 2020-10-09T16:26:13Z

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

JiaLiPassion requested a review from devversion June 15, 2020 03:52

googlebot added the cla: yes label Jun 15, 2020

pullapprove bot requested a review from mhevery June 15, 2020 03:53

JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from f77a02b to 795e34a Compare June 15, 2020 13:54

JiaLiPassion added area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release labels Jun 15, 2020

ngbot bot modified the milestone: needsTriage Jun 15, 2020

JiaLiPassion added the breaking changes label Jun 16, 2020

devversion approved these changes Jun 16, 2020

View reviewed changes

modules/tsconfig-umd.json Outdated Show resolved Hide resolved

JiaLiPassion force-pushed the remove-define-property branch 3 times, most recently from 44e4f32 to ec39950 Compare June 24, 2020 10:58

mhevery approved these changes Jun 29, 2020

View reviewed changes

mhevery added action: merge The PR is ready for merge by the caretaker and removed breaking changes labels Jul 10, 2020

atscott added action: presubmit The PR is in need of a google3 presubmit and removed action: merge The PR is ready for merge by the caretaker labels Jul 13, 2020

atscott added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jul 13, 2020

mhevery added the state: blocked label Jul 15, 2020

JiaLiPassion force-pushed the remove-define-property branch from ec39950 to e199358 Compare July 24, 2020 23:35

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Aug 10, 2020

JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from a159a52 to d9a9e44 Compare September 1, 2020 00:13

JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from 739880e to 57891ac Compare September 1, 2020 04:47

devversion approved these changes Sep 1, 2020

View reviewed changes

AndrewKushnir requested a review from mhevery September 2, 2020 23:47

AndrewKushnir added target: major This PR is targeted for the next major release and removed target: patch This PR is targeted for the next patch release state: blocked labels Sep 2, 2020

JiaLiPassion force-pushed the remove-define-property branch from 57891ac to bd0b527 Compare September 3, 2020 01:38

JiaLiPassion added the action: merge The PR is ready for merge by the caretaker label Sep 3, 2020

AndrewKushnir removed the action: presubmit The PR is in need of a google3 presubmit label Sep 3, 2020

mhevery approved these changes Sep 3, 2020

View reviewed changes

packages/zone.js/lib/browser/define-property.ts Outdated Show resolved Hide resolved

mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 3, 2020

JiaLiPassion force-pushed the remove-define-property branch from bd0b527 to cd760ee Compare September 3, 2020 19:06

JiaLiPassion force-pushed the remove-define-property branch from cd760ee to cfd6217 Compare September 3, 2020 19:08

JiaLiPassion removed the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Sep 3, 2020

atscott closed this in 45a73dd Sep 8, 2020

JiaLiPassion mentioned this pull request Sep 9, 2020

ZoneJS: legacy Object.defineProperty patch breaks configurable descriptor attribute #37432

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Oct 9, 2020

Conversation

JiaLiPassion commented Jun 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

devversion left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhevery commented Jul 10, 2020

Uh oh!

atscott commented Jul 13, 2020

Uh oh!

mhevery commented Jul 15, 2020

Uh oh!

JiaLiPassion commented Jul 16, 2020

Uh oh!

JiaLiPassion commented Jul 24, 2020

Uh oh!

AndrewKushnir commented Jul 28, 2020

Uh oh!

AndrewKushnir commented Sep 1, 2020

Uh oh!

devversion commented Sep 1, 2020

Uh oh!

devversion left a comment

Choose a reason for hiding this comment

Uh oh!

AndrewKushnir commented Sep 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AndrewKushnir commented Sep 2, 2020

Uh oh!

AndrewKushnir commented Sep 3, 2020

Uh oh!

JiaLiPassion commented Sep 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

angular-automatic-lock-bot bot commented Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JiaLiPassion commented Jun 15, 2020 •

edited

Loading

devversion left a comment •

edited

Loading

AndrewKushnir commented Sep 1, 2020 •

edited

Loading

JiaLiPassion commented Sep 3, 2020 •

edited

Loading