Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

Patch captureStackTrace/prepareStackTrace to ZoneAwareError, patch process.nextTick, fix removeAllListeners bug #516

Merged
merged 28 commits into from
Dec 5, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
28 commits
Select commit Hold shift + click to select a range
767f5ef
fix long-stack-trace zone will not render correctly when reject a pro…
JiaLiPassion Aug 17, 2016
1db3876
Merge https://github.com/angular/zone.js
JiaLiPassion Nov 4, 2016
da482b9
fix issue #484 and #491, patch EventEmitter.once and removeAllListeners
JiaLiPassion Nov 9, 2016
4dabc6f
add event emitter once test case
JiaLiPassion Nov 9, 2016
46533fa
prependlistener should add listener at the beginning of the listener …
JiaLiPassion Nov 16, 2016
c4ec5f1
Merge https://github.com/angular/zone.js
JiaLiPassion Nov 16, 2016
2fcaeef
use newest fetch to test
JiaLiPassion Nov 16, 2016
58ee705
Merge remote-tracking branch 'upstream/master'
JiaLiPassion Nov 16, 2016
c5486f8
patch process.nextTick
JiaLiPassion Nov 21, 2016
c832dd6
Merge remote-tracking branch 'upstream/master'
JiaLiPassion Nov 21, 2016
508b3cf
restore ZoneAwareError captureStackTrace function
JiaLiPassion Nov 23, 2016
7c2a616
move captureStackTrace test into node
JiaLiPassion Nov 23, 2016
39644e5
use hasOwnProperty for check captureStackTrace exist or not
JiaLiPassion Nov 23, 2016
058c6a7
change var to const
JiaLiPassion Nov 23, 2016
3f3335f
add process.spec.ts into node_tests.ts target
JiaLiPassion Nov 23, 2016
6938214
add done in process.spec.ts
JiaLiPassion Nov 23, 2016
8046b67
change var to let
JiaLiPassion Nov 23, 2016
285e4c0
add nexttick order case
JiaLiPassion Nov 23, 2016
bedb768
add prepareStackTrace callback to ZoneAwareError
JiaLiPassion Nov 23, 2016
1d3e3d4
fix when EventEmitter removeAllListeners has no event type parameter,…
JiaLiPassion Nov 24, 2016
e011fd3
change some var to let/const, remove unnecessary cancelTask call
JiaLiPassion Nov 25, 2016
5462fcb
modify testcases
JiaLiPassion Nov 25, 2016
44a17fc
remove typo
JiaLiPassion Nov 25, 2016
300b965
use zone.scheduleMicrotask to patch process.nextTick
JiaLiPassion Nov 25, 2016
d55139f
forget use let/const again
JiaLiPassion Nov 25, 2016
4762e65
Merge remote-tracking branch 'upstream/master'
JiaLiPassion Nov 25, 2016
12dd13d
add comment to removeAllListeners patch, and remove useCapturing para…
JiaLiPassion Nov 27, 2016
c9a18c5
update fetch to 2.0.1
JiaLiPassion Dec 5, 2016
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions lib/common/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ function findAllExistingRegisteredTasks(target: any, name: string, capture: bool
const eventTasks: Task[] = target[EVENT_TASKS];
if (eventTasks) {
const result = [];
for (var i = eventTasks.length - 1; i >= 0; i --) {
for (let i = eventTasks.length - 1; i >= 0; i --) {
const eventTask = eventTasks[i];
const data = <ListenerTaskMeta>eventTask.data;
if (data.eventName === name && data.useCapturing === capture) {
Expand Down Expand Up @@ -280,17 +280,24 @@ export function makeZoneAwareRemoveAllListeners(fnName: string, useCapturingPara
const defaultUseCapturing = useCapturingParam ? false : undefined;

return function zoneAwareRemoveAllListener(self: any, args: any[]) {
var eventName = args[0];
var useCapturing = args[1] || defaultUseCapturing;
var target = self || _global;
var eventTasks = findAllExistingRegisteredTasks(target, eventName, useCapturing, true);
if (eventTasks) {
for (var i = 0; i < eventTasks.length; i ++) {
var eventTask = eventTasks[i];
eventTask.zone.cancelTask(eventTask);
}
const target = self || _global;
if (args.length === 0) {
// remove all listeners without eventName
target[EVENT_TASKS] = [];
// we don't cancel Task either, because call native eventEmitter.removeAllListeners will
// will do remove listener(cancelTask) for us
target[symbol]();
return;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
target[symbol](eventName, useCapturing);
const eventName = args[0];
const useCapturing = args[1] || defaultUseCapturing;
// call this function just remove the related eventTask from target[EVENT_TASKS]
findAllExistingRegisteredTasks(target, eventName, useCapturing, true);
// we don't need useCapturing here because useCapturing is just for DOM, and
// removeAllListeners should only be called by node eventEmitter
// and we don't cancel Task either, because call native eventEmitter.removeAllListeners will
// will do remove listener(cancelTask) for us
target[symbol](eventName);
}
}

Expand Down
27 changes: 27 additions & 0 deletions lib/node/node.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import './events';
import './fs';

import {patchTimer} from '../common/timers';
import {patchMethod} from '../common/utils';

const set = 'set';
const clear = 'clear';
Expand All @@ -30,6 +31,7 @@ if (shouldPatchGlobalTimers) {
patchTimer(_global, set, clear, 'Immediate');
}

patchNextTick();

// Crypto
let crypto;
Expand Down Expand Up @@ -83,3 +85,28 @@ if (httpClient && httpClient.ClientRequest) {
}
};
}

function patchNextTick() {
let setNative = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


function scheduleTask(task: Task) {
const args = task.data;
args[0] = function() {
task.invoke.apply(this, arguments);
};
setNative.apply(process, args);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@sjelin sjelin Nov 30, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`
});

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return task;
}

setNative =
patchMethod(process, 'nextTick', (delegate: Function) => function(self: any, args: any[]) {
if (typeof args[0] === 'function') {
const zone = Zone.current;
const task = zone.scheduleMicroTask('nextTick', args[0], args, scheduleTask);
return task;
} else {
// cause an error by calling it directly.
return delegate.apply(process, args);
}
});
}
21 changes: 18 additions & 3 deletions lib/zone.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1264,7 +1264,7 @@ const Zone: ZoneType = (function(global: any) {
/// Skip this frame when printing out stack
blackList,
/// This frame marks zone transition
trasition
transition
};
const NativeError = global[__symbol__('Error')] = global.Error;
// Store the frames which should be removed from the stack frames
Expand Down Expand Up @@ -1304,7 +1304,7 @@ const Zone: ZoneType = (function(global: any) {
if (frameType === FrameType.blackList) {
frames.splice(i, 1);
i--;
} else if (frameType === FrameType.trasition) {
} else if (frameType === FrameType.transition) {
if (zoneFrame.parent) {
// This is the special frame where zone changed. Print and process it accordingly
frames[i] += ` [${zoneFrame.parent.zone.name} => ${zoneFrame.zone.name}]`;
Expand Down Expand Up @@ -1337,6 +1337,21 @@ const Zone: ZoneType = (function(global: any) {
});
}

if (NativeError.hasOwnProperty('captureStackTrace')) {
Object.defineProperty(ZoneAwareError, 'captureStackTrace', {
value: function(targetObject: Object, constructorOpt?: Function) {
NativeError.captureStackTrace(targetObject, constructorOpt);
}
});
}

Object.defineProperty(ZoneAwareError, 'prepareStackTrace', {
get: function() { return NativeError.prepareStackTrace; },
set: function(value) { return NativeError.prepareStackTrace = value; }
});

// Now we need to populet the `blacklistedStackFrames` as well as find the

// Now we need to populet the `blacklistedStackFrames` as well as find the
// run/runGuraded/runTask frames. This is done by creating a detect zone and then threading
// the execution through all of the above methods so that we can look at the stack trace and
Expand Down Expand Up @@ -1364,7 +1379,7 @@ const Zone: ZoneType = (function(global: any) {
// FireFox: Zone.prototype.run@http://localhost:9876/base/build/lib/zone.js:101:24
// Safari: run@http://localhost:9876/base/build/lib/zone.js:101:24
let fnName: string = frame.split('(')[0].split('@')[0];
let frameType = FrameType.trasition;
let frameType = FrameType.transition;
if (fnName.indexOf('ZoneAwareError') !== -1) {
zoneAwareFrame = frame;
}
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,6 @@
"ts-loader": "^0.6.0",
"tslint": "^3.15.1",
"typescript": "^2.0.2",
"whatwg-fetch": "https://github.com/jimmywarting/fetch.git"
"whatwg-fetch": "^2.0.1"
}
}
29 changes: 29 additions & 0 deletions test/node/Error.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

describe('ZoneAwareError', () => {
// If the environment does not supports stack rewrites, then these tests will fail
// and there is no point in running them.
if (!Error['stackRewrite']) return;

it ('should have all properties from NativeError', () => {
let obj: any = new Object();
Error.captureStackTrace(obj);
expect(obj.stack).not.toBeUndefined();
});

it ('should support prepareStackTrace', () => {
(<any>Error).prepareStackTrace = function(error, stack) {
return stack;
}
let obj: any = new Object();
Error.captureStackTrace(obj);
expect(obj.stack[0].getFileName()).not.toBeUndefined();
});
});

48 changes: 48 additions & 0 deletions test/node/events.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,15 @@ describe('nodejs EventEmitter', () => {
expect(emitter.listeners('test').length).toEqual(0);
});
});
it ('should remove All listeners properly even without a type parameter', () => {
zoneA.run(() => {
emitter.on('test', shouldNotRun);
emitter.on('test1', shouldNotRun);
emitter.removeAllListeners();
expect(emitter.listeners('test').length).toEqual(0);
expect(emitter.listeners('test1').length).toEqual(0);
});
});
it ('should remove once listener after emit', () => {
zoneA.run(() => {
emitter.once('test', expectZoneA);
Expand All @@ -119,4 +128,43 @@ describe('nodejs EventEmitter', () => {
emitter.emit('test');
});
});
it ('should trigger removeListener when remove listener', () => {
zoneA.run(() => {
emitter.on('removeListener', function(type, handler) {
zoneResults.push('remove' + type);
});
emitter.on('newListener', function(type, handler) {
zoneResults.push('new' + type);
});
emitter.on('test', shouldNotRun);
emitter.removeListener('test', shouldNotRun);
expect(zoneResults).toEqual(['newtest', 'removetest']);
});
});
it ('should trigger removeListener when remove all listeners with eventname ', () => {
zoneA.run(() => {
emitter.on('removeListener', function(type, handler) {
zoneResults.push('remove' + type);
});
emitter.on('test', shouldNotRun);
emitter.on('test1', expectZoneA);
emitter.removeAllListeners('test');
expect(zoneResults).toEqual(['removetest']);
expect(emitter.listeners('removeListener').length).toBe(1);
});
});
it ('should trigger removeListener when remove all listeners without eventname', () => {
zoneA.run(() => {
emitter.on('removeListener', function(type, handler) {
zoneResults.push('remove' + type);
});
emitter.on('test', shouldNotRun);
emitter.on('test1', shouldNotRun);
emitter.removeAllListeners();
expect(zoneResults).toEqual(['removetest', 'removetest1']);
expect(emitter.listeners('test').length).toBe(0);
expect(emitter.listeners('test1').length).toBe(0);
expect(emitter.listeners('removeListener').length).toBe(0);
});
});
});
82 changes: 82 additions & 0 deletions test/node/process.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,82 @@
/**
* @license
* Copyright Google Inc. All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/

describe('process related test', () => {
let zoneA, result;
beforeEach(() => {
zoneA = Zone.current.fork({name: 'zoneA'});
result = [];
});
it('process.nextTick callback should in zone', (done) => {
zoneA.run(function() {
process.nextTick(() => {
expect(Zone.current.name).toEqual('zoneA');
done();
});
});
});
it('process.nextTick should be excuted before macroTask and promise', (done) => {
zoneA.run(function() {
setTimeout(() => {
result.push('timeout');
}, 0);
process.nextTick(() => {
result.push('tick');
});
setTimeout(() => {
expect(result).toEqual(['tick', 'timeout']);
done();
});
});
});
it ('process.nextTick should be treated as microTask', (done) => {
let zoneTick = Zone.current.fork({
name: 'zoneTick',
onScheduleTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, task: Task): Task => {
result.push(
{
callback: 'scheduleTask',
targetZone: targetZone.name,
task: task.source
});
return parentZoneDelegate.scheduleTask(targetZone, task);
},
onInvokeTask: (parentZoneDelegate: ZoneDelegate, currentZone: Zone, targetZone: Zone, task: Task, applyThis?: any, applyArgs?: any): any => {
result.push(
{
callback: 'invokeTask',
targetZone: targetZone.name,
task: task.source
});
return parentZoneDelegate.invokeTask(targetZone, task, applyThis, applyArgs);
}
});
zoneTick.run(() => {
process.nextTick(() => {
result.push('tick');
});
});
setTimeout(() => {
expect(result.length).toBe(3);
expect(result[0]).toEqual(
{
callback: 'scheduleTask',
targetZone: 'zoneTick',
task: 'nextTick'
});
expect(result[1]).toEqual(
{
callback: 'invokeTask',
targetZone: 'zoneTick',
task: 'nextTick'
}
);
done();
});
})
});
4 changes: 3 additions & 1 deletion test/node_tests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,6 @@
*/

import './node/events.spec';
import './node/fs.spec';
import './node/fs.spec';
import './node/process.spec';
import './node/Error.spec';