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

Jump to bottom

fix(zone.js): defineProperty patch should not swallow error #37582

Closed
wants to merge 2 commits into from
Closed

fix(zone.js): defineProperty patch should not swallow error #37582

wants to merge 2 commits into from

Conversation

JiaLiPassion
Copy link
Contributor

@JiaLiPassion JiaLiPassion commented Jun 15, 2020 •
edited

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

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

@JiaLiPassion JiaLiPassion requested a review from devversion Jun 15, 2020
@pullapprove pullapprove bot requested a review from mhevery Jun 15, 2020
@JiaLiPassion JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from f77a02b to 795e34a Compare Jun 15, 2020
@JiaLiPassion JiaLiPassion added comp: zones target: patch labels Jun 15, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 15, 2020
@ngbot ngbot bot added this to the needsTriage milestone Jun 15, 2020
devversion
devversion approved these changes Jun 16, 2020
View changes
Copy link
Member

@devversion devversion left a comment •
edited

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 Outdated Show resolved Hide resolved
@JiaLiPassion JiaLiPassion force-pushed the remove-define-property branch 3 times, most recently from 44e4f32 to ec39950 Compare Jun 24, 2020
@mhevery
Copy link
Contributor

@mhevery mhevery commented Jul 10, 2020

presubmit

@mhevery mhevery added action: merge and removed breaking changes labels Jul 10, 2020
@atscott atscott added action: presubmit and removed action: merge labels Jul 13, 2020
@atscott
Copy link
Contributor

@atscott atscott commented Jul 13, 2020

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
Copy link
Contributor

@mhevery mhevery commented Jul 15, 2020

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
Copy link
Contributor Author

@JiaLiPassion JiaLiPassion commented Jul 16, 2020

@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
Copy link
Contributor Author

@JiaLiPassion JiaLiPassion commented Jul 24, 2020

@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
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Jul 28, 2020

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

@JiaLiPassion JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from a159a52 to d9a9e44 Compare Sep 1, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 1, 2020

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

@JiaLiPassion JiaLiPassion force-pushed the remove-define-property branch 2 times, most recently from 739880e to 57891ac Compare Sep 1, 2020
@devversion
Copy link
Member

@devversion devversion commented Sep 1, 2020

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
devversion approved these changes Sep 1, 2020
View changes
Copy link
Member

@devversion devversion left a comment

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

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 1, 2020 •
edited

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

@AndrewKushnir AndrewKushnir requested a review from mhevery Sep 2, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 2, 2020

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 AndrewKushnir added target: major and removed target: patch state: blocked labels Sep 2, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Sep 3, 2020

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
Copy link
Contributor Author

@JiaLiPassion JiaLiPassion commented Sep 3, 2020 •
edited

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

@JiaLiPassion JiaLiPassion added the action: merge label Sep 3, 2020
mhevery
mhevery approved these changes Sep 3, 2020
View changes
packages/zone.js/lib/browser/define-property.ts Outdated Show resolved Hide resolved
@JiaLiPassion
fix(zone.js): defineProperty patch should not swallow error
67e055a 
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.
@JiaLiPassion
fix(zone.js): run tests in umd format
cfd6217 
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.
atscott added a commit that referenced this issue Sep 8, 2020
@atscott
fix(zone.js): run tests in umd format (#37582)
40096be 
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
@JiaLiPassion JiaLiPassion mentioned this pull request Sep 9, 2020
Open
@angular-automatic-lock-bot
Copy link

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

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 Oct 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@AndrewKushnir AndrewKushnir

@mhevery mhevery

@devversion devversion

Assignees
No one assigned
Labels
action: merge breaking changes cla: yes comp: zones risk: medium target: major
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

ZoneJS: legacy Object.defineProperty patch breaks configurable descriptor attribute
7 participants
@JiaLiPassion @mhevery @atscott @AndrewKushnir @josephperrott @devversion @googlebot