Patch captureStackTrace/prepareStackTrace to ZoneAwareError, patch process.nextTick, fix removeAllListeners bug by JiaLiPassion · Pull Request #516 · angular/zone.js

JiaLiPassion · 2016-11-23T03:04:25Z

ZoneAwareError didn't add captureStackTrace method, so it will failed when user call Error.captureStackTrace. for example issue 0.7.1 Breaking Error #515
ZoneAwareError didn't add Error.prepareStackTrace callback, so if user add prepareStackTrace callback to Error, the callback will not be executed. for example issue 0.7.1 Breaking Error #515

https://github.com/v8/v8/wiki/Stack-Trace-API

patch process.nextTick with zone.scheduleMicroTask, so process.nextTick can be in zone, and can be handled by ZoneDelegate task related callback(onScheduleTask/onInvokeTask) Patch process.nextTick() #505
modify a typo, FrameType.trasition->FrameType.transition
fix EventEmitter.removeAllListeners bug Incompatabile with nodejs 6.9.1: makeZoneAwareRemoveAllListeners addes 'undefined' arguments to real removeAllListeners calls #518, when call EventEmitter.removeAllListeners without eventName, should remove all eventListener, and add some test cases for EventEmitter.removeListener/newListener.
update fetch to 2.0.1 in package.json

…mise

# Conflicts: # dist/long-stack-trace-zone.js # dist/long-stack-trace-zone.min.js # lib/zone-spec/long-stack-trace.ts # test/zone-spec/long-stack-trace-zone.spec.ts

…moveAllListeners

…array. add test cases for prepend listener and once.

Brooooooklyn · 2016-11-23T03:51:19Z

 }

+// patch process.nextTick
+var nativeNextTick = process.nextTick;


maybe use const better

yes, you are right, thank you.

… should remove all listeners

KrauseStefan

I've looked through the changes, keep in mind that I'm not directly attached to the project so take the feedback for what it is, I do however believe that a few changes are needed before a merge.

KrauseStefan · 2016-11-24T21:31:14Z

+      // remove all listeners without eventName
+      target[EVENT_TASKS] = [];
+      target[symbol]();
+      return;


The real removeAllListeners(...)
return this
This wrapper should do the same
https://github.com/nodejs/node/blob/master/lib/events.js#L404

In typescript I believe you probably want to use either let or const as far as I can tell all except the logic in the loop can use const.

thanks, I will change the var to const.
and about the return this part, it has been handled by https://github.com/angular/zone.js/blob/master/lib/node/events.ts#L14

KrauseStefan · 2016-11-24T21:36:21Z

    var eventName = args[0];
    var useCapturing = args[1] || defaultUseCapturing;
-    var target = self || _global;
    var eventTasks = findAllExistingRegisteredTasks(target, eventName, useCapturing, true);


When all all listeners are removed shouldn't all the eventTasks also be chanceled?
I believe we need to look into how to use findAllExistingRegisteredTasks to get and cancel all eventTasks.

the cancelTask will just call the original EventEmitter.removeListener, I think we don't need to cancel it, because when we call targetsymbol https://github.com/angular/zone.js/pull/516/files#diff-f5e084bcb7c43d977d56c3b791bbca2dR287, the EventEmitter.removeAllListeners will do it for us.

and the cancelTask call here is also not necessary either I think, https://github.com/angular/zone.js/blob/master/lib/common/utils.ts#L290

nakedcity · 2016-11-25T09:35:20Z

Tested it, seems to fix the issue. When are you planning to merge this?

…meter cause it is not needed

sjelin · 2016-11-29T23:35:15Z

+    args[0] = function() {
+      task.invoke.apply(this, arguments);
+    };
+    setNative.apply(process, args);


Shouldn't this be setNative.apply(self, args) using the self variable coming from patchMethod? Not actually a problem since nextTick appears to bind callbacks to global, but still

yeah, we can pass the self form patchMethod to scheduleTask, but here we know it is process , so we can just use process directly, just like patch timer.https://github.com/angular/zone.js/blob/master/lib/common/timers.ts#L30

Do we know it's process?

var nextTick = process.nextTick; nextTick.call(myObject, function() { console.log(this); // If `nextTick` passed its context object directly to // its callback, this would be `myObject`. However, // with the zone.js patch, this would be `process` });

(I edited my code in the above comment, since it used to make no sense)

Yeah, but I can't imagine when we need to call like nextTick.call(myObject, ...), in my understanding we should always call nextTick.call(process, ...)

I also don't think there's ever a good reason to call nextTick with a different context, especially since with the current implementation (and probably all past/future implementations) it would be useless anyway. But it is allowable to use a different context, and so this could be a problem in the future. With setTimeout the DOM spec requires you to call it on an object of type Window or WorkerGlobalScope (i.e. the global scope)

Anyway, I'd prefer to see it the other way, but it honestly doesn't matter

But it is allowable to use a different context, and so this could be a problem in the future. With setTimeout the DOM spec requires you to call it on an object of type Window or WorkerGlobalScope (i.e. the global scope)

I didn't know such kind of usage, thank you for the information. in this PR, I'll just keep it this way just to make the coding style be the same with patchTimer. Thanks again.

sjelin · 2016-11-29T23:37:52Z

 }
+
+function patchNextTick() {
+  let setNative = null;


It seems weird to use this local variable instead of moving scheduleTask inside of patchMethod, but I guess you're trying to limit nested closures?

yeah, I agree, I just keep the coding style to be the same with timer patch, https://github.com/angular/zone.js/blob/master/lib/common/timers.ts#L17

JiaLiPassion added 11 commits August 17, 2016 09:03

fix long-stack-trace zone will not render correctly when reject a pro…

767f5ef

…mise

Merge https://github.com/angular/zone.js

1db3876

# Conflicts: # dist/long-stack-trace-zone.js # dist/long-stack-trace-zone.min.js # lib/zone-spec/long-stack-trace.ts # test/zone-spec/long-stack-trace-zone.spec.ts

fix issue angular#484 and angular#491, patch EventEmitter.once and re…

da482b9

…moveAllListeners

add event emitter once test case

4dabc6f

prependlistener should add listener at the beginning of the listener …

46533fa

…array. add test cases for prepend listener and once.

Merge https://github.com/angular/zone.js

c4ec5f1

use newest fetch to test

2fcaeef

Merge remote-tracking branch 'upstream/master'

58ee705

patch process.nextTick

c5486f8

Merge remote-tracking branch 'upstream/master'

c832dd6

restore ZoneAwareError captureStackTrace function

508b3cf

googlebot added the cla: yes label Nov 23, 2016

JiaLiPassion mentioned this pull request Nov 23, 2016

0.7.1 Breaking Error #515

Closed

JiaLiPassion added 2 commits November 23, 2016 11:20

move captureStackTrace test into node

7c2a616

use hasOwnProperty for check captureStackTrace exist or not

39644e5

Brooooooklyn reviewed Nov 23, 2016

View reviewed changes

JiaLiPassion added 6 commits November 23, 2016 12:49

change var to const

058c6a7

add process.spec.ts into node_tests.ts target

3f3335f

add done in process.spec.ts

6938214

change var to let

8046b67

add nexttick order case

285e4c0

add prepareStackTrace callback to ZoneAwareError

bedb768

JiaLiPassion changed the title ~~Restore captureStackTrace method to ZoneAwareError and patch process.nextTick~~ Patch captureStackTrace/prepareStackTrace to ZoneAwareError and patch process.nextTick Nov 23, 2016

JiaLiPassion mentioned this pull request Nov 24, 2016

Patch process.nextTick() #505

Closed

fix when EventEmitter removeAllListeners has no event type parameter,…

1d3e3d4

… should remove all listeners

JiaLiPassion changed the title ~~Patch captureStackTrace/prepareStackTrace to ZoneAwareError and patch process.nextTick~~ Patch captureStackTrace/prepareStackTrace to ZoneAwareError, patch process.nextTick, fix removeAllListeners bug Nov 24, 2016

JiaLiPassion mentioned this pull request Nov 24, 2016

Incompatabile with nodejs 6.9.1: makeZoneAwareRemoveAllListeners addes 'undefined' arguments to real removeAllListeners calls #518

Closed

KrauseStefan suggested changes Nov 24, 2016

View reviewed changes

JiaLiPassion added 2 commits November 25, 2016 09:25

change some var to let/const, remove unnecessary cancelTask call

e011fd3

modify testcases

5462fcb

JiaLiPassion added 3 commits November 25, 2016 13:57

remove typo

44a17fc

use zone.scheduleMicrotask to patch process.nextTick

300b965

forget use let/const again

d55139f

JiaLiPassion mentioned this pull request Nov 25, 2016

ZoneAwareError captureStackTrace Node v7.2.0 not defined #520

Closed

JiaLiPassion added 2 commits November 26, 2016 00:06

Merge remote-tracking branch 'upstream/master'

4762e65

add comment to removeAllListeners patch, and remove useCapturing para…

12dd13d

…meter cause it is not needed

sjelin reviewed Nov 29, 2016

View reviewed changes

update fetch to 2.0.1

c9a18c5

JiaLiPassion mentioned this pull request Dec 5, 2016

feat(Zone): check global for rootZoneSpec #521

Closed

sjelin mentioned this pull request Dec 5, 2016

zone.js might not intercept some native node functions sjelin/zoned-scheduler#1

Closed

mhevery merged commit c36c0bc into angular:master Dec 5, 2016

troyt-42 mentioned this pull request Apr 13, 2023

Zone.js process.nextTick is not patched angular/angular#49830

Closed

Conversation

JiaLiPassion commented Nov 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Brooooooklyn Nov 23, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

KrauseStefan left a comment

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!

nakedcity commented Nov 25, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sjelin Nov 30, 2016 • 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

JiaLiPassion commented Nov 23, 2016 •

edited

Loading

Brooooooklyn Nov 23, 2016 •

edited

Loading

nakedcity commented Nov 25, 2016 •

edited

Loading

sjelin Nov 30, 2016 •

edited

Loading