Make fetch() zone-aware without triggering extra requests or uncatchable errors. by MichaelBuerge · Pull Request #622 · angular/zone.js

MichaelBuerge · 2017-02-01T13:00:25Z

Previous implementation called fetch() during setup in order to obtain a result promise from it so its constructor could be patched to make it zone-aware.
This had two problems:

Calling fetch() would trigger an unwanted (and unnecessary) HTTP-request.
Chrome threw an uncatchable error, which means with open dev tools will halt execution and enter the debugger (which is hugely annoying, as it happens on every page load).

This change replaces fetch() with a wrapper function that lazily ensures the result returned by fetch() is zone-aware.

…ensuring a ZoneAwarePromise is returned. Use it on fetch().

… constructor.

googlebot · 2017-02-01T13:00:32Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot · 2017-02-01T13:00:32Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

MichaelBuerge · 2017-02-01T13:05:17Z

CLA signed.

googlebot · 2017-02-01T13:05:19Z

CLAs look good, thanks!

googlebot · 2017-02-01T13:05:19Z

CLAs look good, thanks!

MichaelBuerge · 2017-02-01T13:10:18Z

Addresses #446

JiaLiPassion · 2017-02-01T16:16:48Z

@MichaelBuerge , please read DEVELOPER.md, before commit part, so you can check your commit. Currently it seems your build failed with tslint part.

MichaelBuerge · 2017-02-01T17:59:21Z

@JiaLiPassion Apologies for missing the linter. Everything should be fine now (npm run ci goes through).

JiaLiPassion · 2017-02-02T01:42:34Z

@MichaelBuerge , you are welcome, could you check my review comment? especially whether the first promise is patched or not one.

MichaelBuerge · 2017-02-02T11:38:48Z

@JiaLiPassion I can't find your review comment. As of now, the pull request page still says "no reviews". (Am I looking in the wrong place?)

However, guessing from your comment here, your question seems to be whether the first time the patched fetch() function is called its result promise is properly patched.
That is indeed the case, as the constructor's prototype is modified before any caller can get a hold of the returned promise. Also, the fetch tests (in test/common/Promise.spec.ts) would not pass with an unpatched result promise.
Does this address your concern?

JiaLiPassion · 2017-02-01T13:35:09Z

+  const symbolThenPatched = __symbol__('thenPatched');
+
+  function patchThen(Ctor) {
+    let proto = Ctor.prototype;


I think we should use const here for proto and originalThen

JiaLiPassion · 2017-02-01T13:37:37Z

+    proto[symbolThen] = originalThen;
+
+    Ctor.prototype.then = function(onResolve, onReject) {
+      let wrapped = new ZoneAwarePromise(


also const here

JiaLiPassion · 2017-02-02T11:51:21Z

@MichaelBuerge, sorry I forget to push submit review button...

That is indeed the case, as the constructor's prototype is modified before any caller can get a hold of the returned promise. Also, the fetch tests (in test/common/Promise.spec.ts) would not pass with an unpatched result promise.
Does this address your concern?

yeah, you are right! thank you for explaining this, I didn't realize that.

JiaLiPassion

thank you!

MichaelBuerge · 2017-02-02T12:52:34Z

My pleasure!
I assume there's a process in place, but if there's anything left do so this gets merged eventually and the associated issue (#446) closed please let me know.

MichaelBuerge · 2017-02-08T00:00:46Z

Also addresses #476

MichaelBuerge added 7 commits January 22, 2017 22:05

Instrument with probePromise()

530a808

Merge remote-tracking branch 'upstream/master'

4ffd1d6

De-instrument (probePromise).

4988bf0

Introduce helper function to wraps any function returning a promise, …

17258af

…ensuring a ZoneAwarePromise is returned. Use it on fetch().

Minor readability improvement.

48e0e39

No need to wrap result promise in ZoneAwarePromise after patching its…

fe4625a

… constructor.

Remove promise prober from karma build conf.

3046ec8

googlebot added the cla: no label Feb 1, 2017

googlebot added cla: yes and removed cla: no labels Feb 1, 2017

MichaelBuerge mentioned this pull request Feb 1, 2017

#445 fix causes uncaught promise rejection in chrome #446

Closed

Address linter issues.

1d126f1

JiaLiPassion reviewed Feb 2, 2017

View reviewed changes

Use const.

e3d1640

JiaLiPassion approved these changes Feb 2, 2017

View reviewed changes

JiaLiPassion mentioned this pull request Feb 5, 2017

HTTPS security is compromised by null in Edge #476

Closed

mhevery merged commit 6731ad0 into angular:master Feb 7, 2017

JiaLiPassion mentioned this pull request Feb 13, 2017

fetch() with no arguments check does an ajax call in safari technology preview #635

Closed

vladimir-kotikov mentioned this pull request Mar 10, 2017

App stops on 'debugger' breakpoint in polyfill.js microsoft/vscode-cordova#251

Closed

Conversation

MichaelBuerge commented Feb 1, 2017

Uh oh!

googlebot commented Feb 1, 2017

Uh oh!

googlebot commented Feb 1, 2017

Uh oh!

MichaelBuerge commented Feb 1, 2017

Uh oh!

googlebot commented Feb 1, 2017

Uh oh!

googlebot commented Feb 1, 2017

Uh oh!

MichaelBuerge commented Feb 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

MichaelBuerge commented Feb 1, 2017

Uh oh!

JiaLiPassion commented Feb 2, 2017

Uh oh!

MichaelBuerge commented Feb 2, 2017

Uh oh!

JiaLiPassion Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

MichaelBuerge Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion Feb 1, 2017

Choose a reason for hiding this comment

Uh oh!

MichaelBuerge Feb 2, 2017

Choose a reason for hiding this comment

Uh oh!

JiaLiPassion commented Feb 2, 2017

Uh oh!

JiaLiPassion left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelBuerge commented Feb 2, 2017

Uh oh!

MichaelBuerge commented Feb 8, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

MichaelBuerge commented Feb 1, 2017 •

edited

Loading

JiaLiPassion commented Feb 1, 2017 •

edited

Loading