-
Notifications
You must be signed in to change notification settings - Fork 409
fix(error): fix #618, ZoneAwareError should copy Error's static properties as well #647
Conversation
b55b4ad
to
f3b8885
Compare
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.
I think this SHA should be reverted?
@@ -9,6 +9,7 @@ | |||
module.exports = function (config) { | |||
require('./karma-base.conf.js')(config); | |||
config.files.push('build/test/wtf_mock.js'); | |||
config.files.push('build/test/custom_error.js'); |
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.
Why do we need this?
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.
I add this file just to simulate a Error with customized method before patched by zone.js. so I can test the customized method is still valid after zone.js patch Error with ZoneAwareError.
@@ -0,0 +1,14 @@ | |||
/** |
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.
I don't see this file being used.
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.
@@ -8,6 +8,7 @@ | |||
|
|||
// Must be loaded before zone loads, so that zone can detect WTF. | |||
import './wtf_mock'; | |||
import './custom_error'; |
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.
Why do we need this?
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.
This PR is failing travis, can you get it rebased and green? |
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
1 similar comment
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
CLAs look good, thanks! |
CLAs look good, thanks! |
fix #618, ZoneAwareError copy the Error's prototype properties , but not the properties of Error itself (static one)
for example, if other library define some custom function to Native Error
then after zone.js patch Error, the func of Error will be lost.
in this PR, I also copy those properties from NativeError.