feat(error): remove zone internal stack frames in error.stack #632
Conversation
JiaLiPassion
force-pushed
the
simplestack
branch
8 times, most recently
from
ab32b37
to
ae3543e
Compare
lib/helper/zone-helper.ts
Outdated
| @@ -0,0 +1,147 @@ | |||
| /** | |||
There was a problem hiding this comment.
Why is this separate file? Why not just fold it to the error handling in Zone.ts which already filters out some stack frames. We could just expand it to filter all?
I don't think most people would use this as it would require set up. I think having this as part of Zone.ts would make it so that everyone would benefit.
There was a problem hiding this comment.
Thank for the review. I will put it into zone.ts.
JiaLiPassion
force-pushed
the
simplestack
branch
from
ae3543e
to
67e17b6
Compare
|
@mhevery , I removed the zone-helper, and add the logic to long-stack-trace, because the detect logic need long-stack-trace zone spec. Please review. |
JiaLiPassion
force-pushed
the
simplestack
branch
from
67e17b6
to
c50f4fe
Compare
test/helper/zone-helper.spec.ts
Outdated
|
|
||
| it('test getNonZoneAwareStack', errorTest(() => { | ||
| setTimeout(() => { | ||
| throw new Error('test error'); |
There was a problem hiding this comment.
What is being asserted here?
There was a problem hiding this comment.
I write a help method errorTest which check the error stack, I will change the name of errorTest to assertStackDoesNotContainZoneFramesTest
test/helper/zone-helper.spec.ts
Outdated
| }; | ||
| }; | ||
|
|
||
| it('test getNonZoneAwareStack', errorTest(() => { |
There was a problem hiding this comment.
it('should do something' Saying it('test getNonZoneAwareStack does not say what is the expected behavior. Always write tests with what should happen, not what method you are testing.
There was a problem hiding this comment.
Always write tests with what should happen, not what method you are testing.
I will remember it, thank you.
test/helper/zone-helper.spec.ts
Outdated
| }, 10); | ||
| })); | ||
|
|
||
| it('test promise getNonZoneAwareStack', (done) => { |
There was a problem hiding this comment.
example: it('should ensure that rejected errors should not have zone frames visible',
There was a problem hiding this comment.
Yes, you are right, I should write what to test.
test/helper/zone-helper.spec.ts
Outdated
| const task = Zone.current.scheduleEventTask('errorEvent', () => { | ||
| throw new Error('test error'); | ||
| }, null, () => null, null); | ||
| task.invoke(); |
There was a problem hiding this comment.
no assertion, and the test does not tell me what should happen.
There was a problem hiding this comment.
I write a help method errorTest which check the error stack, I will change the name of errorTest to assertStackDoesNotContainZoneFramesTest
test/helper/zone-helper.spec.ts
Outdated
|
|
||
| it('test longStackTrace getNonZoneAwareStack', errorTest(() => { | ||
| const task = | ||
| Zone.current.fork(Zone['longStackTraceZoneSpec']).scheduleEventTask('errorEvent', () => { |
There was a problem hiding this comment.
how is this test different from previous test?
There was a problem hiding this comment.
longStackTraceZoneSpec has several stack frames such as 'new LongStackTrace' should also be trimmed, I will change the test case name.
test/helper/zone-helper.spec.ts
Outdated
| task.invoke(); | ||
| })); | ||
|
|
||
| it('test custom zoneSpec getNonZoneAwareStack', errorTest(() => { |
There was a problem hiding this comment.
what is the expectation of this test?
There was a problem hiding this comment.
I write a help method errorTest which check the error stack contains zone aware frames or not, I will change the name of errorTest to assertStackDoesNotContainZoneFramesTest.
test/helper/zone-helper.spec.ts
Outdated
| 'ZoneDelegate.invokeTask', 'ZoneTask.invoke', 'zoneAwareAddListener', 'drainMicroTaskQueue' | ||
| ]; | ||
|
|
||
| const expectStack = function(err: Error) { |
There was a problem hiding this comment.
Better name should be. assertStakDoesNotContainZoneFrames
There was a problem hiding this comment.
Thanks, I will use this name
test/helper/zone-helper.spec.ts
Outdated
| } | ||
| const frames = simpleStack.split('\n'); | ||
| for (let i = 0; i < frames.length; i++) { | ||
| expect(zoneAwareFrames.filter(f => frames[i].indexOf(f) !== -1).length).toBe(0); |
There was a problem hiding this comment.
When this fails it will say was 5 expected 0 which is not useful.
expect(extraFrames).toEqual([]);
is better since the error would be
was ['someFrame] expected []
There was a problem hiding this comment.
you are right, I will update it.
test/helper/zone-helper.spec.ts
Outdated
| return parentDelegate.scheduleTask(targetZone, task); | ||
| }, | ||
| onHandleError: (parentDelegate, currentZone, targetZone, error) => { | ||
| parentDelegate.handleError(targetZone, error); |
There was a problem hiding this comment.
Also have additional test, which shows that this works even when calling it using throw Error(...) (no new) as I think additional frames appear than.
There was a problem hiding this comment.
Yes, I will add the test case throw Error(...) cases
lib/zone-spec/long-stack-trace.ts
Outdated
| @@ -136,6 +136,151 @@ function captureStackTraces(stackTraces: string[][], count: number): void { | |||
| } | |||
| } | |||
|
|
|||
| const zoneAwareStackFrames = {}; | |||
There was a problem hiding this comment.
I think we should just roll this into the existing stack frame filtering in the zone.ts I don't think that having it in long-stack-trace is the right place. If it is in the zone.ts than long-stack-trace will get it automatically.
There was a problem hiding this comment.
Yes, sure, I put it into longStackTrace because it can catch the stack frames for onScheduleTask, and I realize I can just create a error in onScheduleTask callback, I will move those code into zone.ts
JiaLiPassion
force-pushed
the
simplestack
branch
2 times, most recently
from
738db56
to
4df13d3
Compare
|
@mhevery , I have refactor the code, please review, thank you. |
mhevery
force-pushed
the
master
branch
12 times, most recently
from
b55b4ad
to
f3b8885
Compare
karma-dist.conf.js
Outdated
| @@ -17,5 +17,6 @@ module.exports = function (config) { | |||
| config.files.push('dist/sync-test.js'); | |||
| config.files.push('dist/task-tracking.js'); | |||
| config.files.push('dist/wtf.js'); | |||
| config.files.push('dist/zone-helper.js'); | |||
There was a problem hiding this comment.
Where does this come from? We don't have zone-helper
There was a problem hiding this comment.
I forget to remove it.
test/common/Error.spec.ts
Outdated
| @@ -158,7 +158,7 @@ describe('ZoneAwareError', () => { | |||
| }); | |||
|
|
|||
| it('should show zone names in stack frames and remove extra frames', () => { | |||
| const rootZone = getRootZone(); | |||
| const rootZone = Zone.root; | |||
There was a problem hiding this comment.
Could we add a test which says that new Error('foo') and Error(foo`) both get filtered correctly? I have seen some issues in the past there.
There was a problem hiding this comment.
I have added the test case of Error('foo'), and because Error without new will cause different stack frames, so I also add detect logic in zone.ts.
lib/zone.ts
Outdated
| 'long-stack-trace' | ||
| ]; | ||
|
|
||
| Object.defineProperty(ZoneAwareError, 'getNonZoneAwareStack', { |
There was a problem hiding this comment.
-
Why is this called
getNonZoneAwareStack? what is returned is ZoneAware.
Why do we even need this method? -
Why is this
Object.definePropertyrather than simple function assignment?
There was a problem hiding this comment.
yes, the method name is confused, it is ZoneAware but without the Zone related stack frames. such as Zone.run, Zone.scheduleXXXTask, ZoneDelegate.invoke...
I will change it to getSimplifiedStack, is that ok?
And use Object.defineProperty here is just to keep the coding style to be consistent with other property such as prepareStackTrace, captureStackTrace.
There was a problem hiding this comment.
-
Why do we need the method than?
.stackproperty has the frames removed and.originalStackhas the extra frames. Could you explain the motivation behindgetNonZoneAwareStack -
I think
Object.definePropertyis confusing. I would just drop it.
There was a problem hiding this comment.
The motivation is currently the .stack property still has a lot of information related with Zone,
such as Zone.runTask, ZoneDelegate.invokeTask and so on, ZoneAwareError only remove a part of the stack frames which Zone related. for example
Zone.current.fork({name: 'test'}).run(function(){
document.getElementById('ok').addEventListener('click', function() {
console.log(new Error('test').stack);
});
});will get
Error: test
at HTMLButtonElement.<anonymous> (test4.html:30) [test]
at Zone.runTask (zone.js:166) [<root> => test]
at HTMLButtonElement.ZoneTask.invoke (zone.js:418) [<root>]
throw a error in button click will get the .stack
There was a problem hiding this comment.
I have remove the getNonZoneAwareStack and move the logic to .stack property.
JiaLiPassion
force-pushed
the
simplestack
branch
4 times, most recently
from
ddeec4a
to
7504ec6
Compare
JiaLiPassion
force-pushed
the
simplestack
branch
2 times, most recently
from
8b07351
to
0db2680
Compare
JiaLiPassion
force-pushed
the
simplestack
branch
from
0db2680
to
bbd9ac3
Compare
JiaLiPassion
changed the title
…r#632) This feature was reverted because it was causing a performance regression. The setup of all of the stack traces added about 20-25ms to the total time of the Zone initialization. This was a bit more than 50% of overall time zone needed to initialize. While this is a useful feature, the performance cost does not justify its cost.
…r#632) This feature was reverted because it was causing a performance regression. The setup of all of the stack traces added about 20-25ms to the total time of the Zone initialization. This was a bit more than 50% of overall time zone needed to initialize. While this is a useful feature, the performance cost does not justify its cost.
…r#632) This feature was reverted because it was causing a performance regression. The setup of all of the stack traces added about 20-25ms to the total time of the Zone initialization. This was a bit more than 50% of overall time zone needed to initialize. While this is a useful feature, the performance cost does not justify its cost.
…690) This feature was reverted because it was causing a performance regression. The setup of all of the stack traces added about 20-25ms to the total time of the Zone initialization. This was a bit more than 50% of overall time zone needed to initialize. While this is a useful feature, the performance cost does not justify its cost.
1.zone.js patches most basic APIs, so when we want to debug, we will see a lot of zone related stack traces, for example, in example/basic.html, the simple event listener stack trace will look like this.
It is a little difficult to find out which is the application's stack, so in this PR, we reduce the stack frames belong to zone internal.
Basically, only user's code will be left here. not all, but most.
to access the original stack, just call
Another changes is in IE, error.stack will be undefined when constructed, and only have the stack
when throw.
https://docs.microsoft.com/en-us/scripting/javascript/reference/stack-property-error-javascript
in this PR, the stack will be there when constructed.
fix typo in zone.js
fix test cases for fs.spec.ts to remove warning.