-
Notifications
You must be signed in to change notification settings - Fork 407
Patch captureStackTrace/prepareStackTrace to ZoneAwareError, patch process.nextTick, fix removeAllListeners bug #516
Changes from all commits
767f5ef
1db3876
da482b9
4dabc6f
46533fa
c4ec5f1
2fcaeef
58ee705
c5486f8
c832dd6
508b3cf
7c2a616
39644e5
058c6a7
3f3335f
6938214
8046b67
285e4c0
bedb768
1d3e3d4
e011fd3
5462fcb
44a17fc
300b965
d55139f
4762e65
12dd13d
c9a18c5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ import './events'; | |
import './fs'; | ||
|
||
import {patchTimer} from '../common/timers'; | ||
import {patchMethod} from '../common/utils'; | ||
|
||
const set = 'set'; | ||
const clear = 'clear'; | ||
|
@@ -30,6 +31,7 @@ if (shouldPatchGlobalTimers) { | |
patchTimer(_global, set, clear, 'Immediate'); | ||
} | ||
|
||
patchNextTick(); | ||
|
||
// Crypto | ||
let crypto; | ||
|
@@ -83,3 +85,28 @@ if (httpClient && httpClient.ClientRequest) { | |
} | ||
}; | ||
} | ||
|
||
function patchNextTick() { | ||
let setNative = null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems weird to use this local variable instead of moving There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't this be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we know it's 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`
}); There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, ...) There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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); | ||
} | ||
}); | ||
} |
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(); | ||
}); | ||
}); | ||
|
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(); | ||
}); | ||
}) | ||
}); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
orconst
as far as I can tell all except the logic in the loop can useconst
.There was a problem hiding this comment.
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