Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(BaseError): Don't expect super() to return a new Error object. #12600

Merged
merged 1 commit into from Dec 14, 2016
Merged

refactor(BaseError): Don't expect super() to return a new Error object. #12600

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion modules/@angular/facade/src/errors.ts
View file
Open in desktop
Expand Up @@ -18,9 +18,12 @@ export class BaseError extends Error {
_nativeError: Error;

constructor(message: string) {
super(message);
// Errors don't use current this, instead they create a new instance.
// We have to do forward all of our api to the nativeInstance.
const nativeError = super(message) as any as Error;
// TODO(bradfordcsmith): Remove this hack when
// google/closure-compiler/issues/2102 is fixed.
const nativeError = new Error(message) as any as Error;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this change calls Error twice, which means that stack trace capturing (which is expensive) is done twice as well.

What are the reasons for this change as it makes the code slower.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original code depended on accidental, incorrect behavior that closure-compiler and the TypeScript compiler happened to share. The super() keyword call was transpiled to basically Error.call(this, message), which ignores the this parameter and just returns a new Error object.

I needed to fix this behavior.
google/closure-compiler#2102

as part of the effort to make super()'s behavior match the ES spec.
google/closure-compiler#1669

I've now made these fixes. If my code change in errors.ts is reverted, then nativeError will be an alias for this, and the getters and setters will become infinite loops when this code is used with the tool chain TypeScript -> (TypeScript compiler) -> ES6 code -> (closure-compiler) -> ES5 code.

Personally, I think TypeScript should do special-case handling of Error classes like closure-compiler now does. Then all of this hacky workaround in the errors.ts code could go away entirely.
I said so in another angular issue, but I don't have the number handy.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, makes sense.

this._nativeError = nativeError;
}

Expand Down