feat: add zone configuration to typescript interface by JiaLiPassion · Pull Request #35329 · angular/angular

JiaLiPassion · 2020-02-11T10:16:34Z

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:

Add all zone.js configuration to interface, so we can define those flags in polyfills.ts like this.

const config = window as ZoneGlobalConfiguration;
config.__Zone_disable_requestAnimationFrame = true;

googlebot · 2020-02-11T10:17:02Z

☹️ Sorry, but only Googlers may change the label cla: yes.

JiaLiPassion · 2020-02-15T10:27:10Z

@mhevery, thanks for the review, I have updated the doc.
@kapunahelewong , could you also help me to check the comments in the src file? Basically I will use the same text to write a doc later to explain all the configurations in aio. Thanks.

kapunahelewong

Just small adjustments. Stoppping here to see if all the comments will go through.

Note: For context, last time I had too many comments and had to duplicate my work in the file itself because github wouldn't post my review.

kapunahelewong · 2020-02-17T14:09:34Z

Suggested change

* Zone.js will not monkey patch Object.defineProperty API and will not

* Zone.js does not monkey patch the `Object.defineProperty` API and does not

kapunahelewong · 2020-02-17T14:10:02Z

Suggested change

* try to modify desc.configurable to true.

* try to modify desc.configurable to `true`.

kapunahelewong · 2020-02-17T14:10:36Z

Suggested change

* Disable registerElement monkey patch.

* Disable the `registerElement` monkey patch.

kapunahelewong · 2020-02-17T14:11:09Z

Suggested change

* Only available in legacy bundle (dist/zone.js), this defineProperty module will not be

* Only available in the legacy bundle (`dist/zone.js`), `this defineProperty` module is not

kapunahelewong · 2020-02-17T14:11:22Z

Suggested change

* available in evergreen bundle (zone-evergreen.js).

* available in the evergreen bundle (`zone-evergreen.js`).

kapunahelewong

Just small adjustments. Stoppping here to see if all the comments will go through.

Note: For context, last time I had too many comments and had to duplicate my work in the file itself because github wouldn't post my review.

JiaLiPassion · 2020-02-20T03:21:48Z

@kapunahelewong , thank you for the detailed review, I have updated the PR, please review.

mhevery

With nits.

mhevery · 2020-02-24T21:51:03Z

Why is this called timers not setTimeout? What other times are we disabling?

in this module, we disable setTimeout/setInterval/setImmediate.

mhevery · 2020-02-24T21:53:26Z

really why is registerElement not available in evergreen?

Because registerElement is deprecated, and we now only including customElement.define monkey patch for web component.

mhevery · 2020-02-24T21:54:32Z

Same here, why is it not available in evergreen Could you add explanation to the reasoning behind it?

Sure, I will add more doc here, the reason we don't monkey patch legacy EventTarget like APIs here because in modern browsers, the EventTarget is available, so we only need to monkey patch EventTarget, we don't need to monkey patch such as HTMLDivElement class one by one.

mhevery · 2020-02-24T21:56:05Z

Are there other timeres besides setTimeout? if not why not __Zone_disable_setTimeout?

in nodejs, we also patch setTimeout/setInterval/setImmediate.

ngbot · 2020-02-26T05:06:16Z

I see that you just added the PR action: merge label, but the following checks are still failing:
     status "ci/circleci: legacy-unit-tests-saucelabs" is failing
     status "google3" is pending
     missing required status "ci/circleci: publish_snapshot"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

JiaLiPassion · 2020-02-26T23:58:45Z

@mhevery, I have rebased the PR, the CI is red because the early other PRs changed the payload size in aio, #35702

mhevery · 2020-02-27T19:51:36Z

presubmit

atscott · 2020-02-27T22:08:18Z

Hi @JiaLiPassion, there was a conflict with the patch branch so I modified this PR to target master only. Can you create a follow-up PR that targets patch-only?

JiaLiPassion · 2020-02-28T15:19:10Z

@atscott , got it, thanks, I will create a separate PR, and will DM you for the details.

aikithoughts

Edit pass for readability.

kara · 2020-03-04T00:01:45Z

@JiaLiPassion Can you open a follow-up PR to address @aikidave's comments?

JiaLiPassion · 2020-03-04T00:31:15Z

@kara, sure, will do it now. thanks.

tonivj5 · 2020-03-26T12:16:05Z

+   * Users can achieve this goal by defining `__zone_symbol__UNPATCHED_EVENTS = ['scroll',
+   * 'mousemove'];` before imporing `zone.js`.
+   */
+  __zone_symbol__UNPATCHED_EVENTS?: boolean;


@JiaLiPassion __zone_symbol__UNPATCHED_EVENTS and __zone_symbol__PASSIVE_EVENTS should be a string[] instead of boolean, shouldn't?

Thanks for add types to zone.js, it's awesome 🎉

Oh, yes, you are right, thanks for finding this issue, will create PR to resolve it.

angular-automatic-lock-bot · 2020-04-26T16:06:00Z

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

pullapprove Bot requested a review from mhevery February 11, 2020 10:16

googlebot added the cla: yes label Feb 11, 2020

JiaLiPassion added area: zones Issues related to zone.js target: patch This PR is targeted for the next patch release and removed cla: yes labels Feb 11, 2020

ngbot Bot added this to the needsTriage milestone Feb 11, 2020

googlebot added the cla: yes label Feb 11, 2020

mhevery suggested changes Feb 11, 2020

View reviewed changes

Comment thread packages/zone.js/lib/zone.configuration.api.ts Outdated

Comment thread packages/zone.js/lib/zone.configuration.api.ts Outdated

Comment thread packages/zone.js/lib/zone.configuration.api.ts Outdated

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 Feb 11, 2020

JiaLiPassion force-pushed the zone-module-api branch from 1d87c20 to 466366e Compare February 15, 2020 05:31

JiaLiPassion requested review from kapunahelewong and mhevery February 15, 2020 10:25

kapunahelewong reviewed Feb 17, 2020

View reviewed changes

JiaLiPassion force-pushed the zone-module-api branch from 466366e to 195777c Compare February 20, 2020 03:21

JiaLiPassion force-pushed the zone-module-api branch from 195777c to 2f71a72 Compare February 20, 2020 03:22

JiaLiPassion requested a review from kapunahelewong February 20, 2020 04:39

kapunahelewong approved these changes Feb 20, 2020

View reviewed changes

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 Feb 20, 2020

JiaLiPassion force-pushed the zone-module-api branch 2 times, most recently from bac83f6 to 807f3c9 Compare February 24, 2020 04:23

mhevery approved these changes Feb 24, 2020

View reviewed changes

JiaLiPassion force-pushed the zone-module-api branch 2 times, most recently from 33b6100 to 17263e1 Compare February 25, 2020 14:36

mhevery added the action: merge The PR is ready for merge by the caretaker label Feb 26, 2020

JiaLiPassion force-pushed the zone-module-api branch from 17263e1 to a2e0647 Compare February 26, 2020 21:56

JiaLiPassion force-pushed the zone-module-api branch from a2e0647 to 1e081c4 Compare February 26, 2020 22:46

feat: define all zone.js configurations to typescript interfaces

404a2a5

JiaLiPassion force-pushed the zone-module-api branch from 1e081c4 to 404a2a5 Compare February 27, 2020 10:31

atscott 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 labels Feb 27, 2020

atscott closed this in 03d88c7 Feb 27, 2020

AndrewKushnir mentioned this pull request Mar 3, 2020

build: adding a script to compare commits in master and stable branches #35130

Closed

3 tasks

aikithoughts reviewed Mar 3, 2020

View reviewed changes

JiaLiPassion mentioned this pull request Mar 4, 2020

docs(zone.js): update coments of zone configuration API definition #35849

Closed

12 tasks

tonivj5 reviewed Mar 26, 2020

View reviewed changes

angular-automatic-lock-bot Bot locked and limited conversation to collaborators Apr 26, 2020

	* Zone.js will not monkey patch Object.defineProperty API and will not
	* Zone.js does not monkey patch the `Object.defineProperty` API and does not

	* try to modify desc.configurable to true.
	* try to modify desc.configurable to `true`.

	* Disable registerElement monkey patch.
	* Disable the `registerElement` monkey patch.

	* Only available in legacy bundle (dist/zone.js), this defineProperty module will not be
	* Only available in the legacy bundle (`dist/zone.js`), `this defineProperty` module is not

	* available in evergreen bundle (zone-evergreen.js).
	* available in the evergreen bundle (`zone-evergreen.js`).

Conversation

JiaLiPassion commented Feb 11, 2020

PR Checklist

PR Type

Uh oh!

googlebot commented Feb 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JiaLiPassion commented Feb 15, 2020

Uh oh!

kapunahelewong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kapunahelewong left a comment

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Feb 20, 2020

Uh oh!

mhevery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ngbot Bot commented Feb 26, 2020

Uh oh!

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

Uh oh!

Uh oh!

mhevery commented Feb 27, 2020

Uh oh!

atscott commented Feb 27, 2020

Uh oh!

JiaLiPassion commented Feb 28, 2020

Uh oh!

aikithoughts left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JiaLiPassion commented Feb 26, 2020 •

edited

Loading