refactor(Error): Rewrite custom derived errors with Object.create()

[tetsuharuohzeki]

Description:

• This is the alternative proposal to fix Custom error object is no longer typeof Error from TypeScript 2.1 #2178 with rewriting custom derived errors by Object.create()'s classical inheritance by hand.
  • This approach is based on TypeScript's definitions for build-in object.
  • By this change, we can upgrade TypeScript 2.1 or later without dropping support for IE9/IE10 for the future.
• This is not a breaking change because this patch creates an inheritance by hand instead of TypeScript's transformer.

Related issue (if exists):

#2178

[tetsuharuohzeki]

@kwonoj how about this?

[coveralls]

Coverage increased (+0.001%) to 97.691% when pulling 579cc46 on saneyuki:rewrite-custom-errors into 6922b16 on ReactiveX:master.

[coveralls]

Coverage increased (+0.002%) to 97.692% when pulling 49127be on saneyuki:rewrite-custom-errors into 6922b16 on ReactiveX:master.

[kwonoj]

Yes, this seems a working solution. I'll leave opened to get second eye for review.

[kwonoj]

Personally, I'd like to land #2202 (and following PR) prior to any-targeting-master changes to enable type inference-validation on each changes. Though this looks solid (and local verification works), always better to have test supports.

[coveralls]

Coverage increased (+0.003%) to 97.693% when pulling 2facaf9 on saneyuki:rewrite-custom-errors into 6922b16 on ReactiveX:master.

[tetsuharuohzeki]

I added some test cases for this change.

[coveralls]

Coverage increased (+0.003%) to 97.693% when pulling b35e05a on saneyuki:rewrite-custom-errors into 6922b16 on ReactiveX:master.

[benlesh]

Interesting. I played around with this same approach for a few minutes and TS 2.1 didn't like it when I tried. Heh. Looks good to me, though.

[DanielRosenwasser]

For the record, you can create an intermediate class that "does the right thing" by not explicitly returning from the constructor itself. You could write it as an ES5-style class (much the same way the old code would be compiled), and then derive your classes from that class. That way, much less of the code actually has to be rewritten.

var __extends = (this && this.__extends) || function (d, b) {
    for (var p in b) if (b.hasOwnProperty(p)) d[p] = b[p];
    function __() { this.constructor = d; }
    d.prototype = b === null ? Object.create(b) : (__.prototype = b.prototype, new __());
};

export interface ShimErrorStatic {
    new (message?: string): ShimError;
}
export interface ShimError extends Error {}

export const ShimError: ShimErrorStatic = (function (_super) {
    __extends(ShimError, _super);
    function ShimError(message) {
        _super.apply(this, arguments);
        // ensure that 'message' gets set correctly - wasn't case with old implementation.
        this.message = message;
    }
    return ShimError;
}(Error)) as any;

The only downside is that if you're compiling to ES2015, this winds up being an extra layer that you may find undesirable.

[kwonoj]

compiling to ES2015, this winds up being an extra layer

@DanielRosenwasser would you able to share how much will it be? I expect ES15 package will be published eventually, so if those are unacceptable amount probably would better to go with existing changes in this PR.

[DanielRosenwasser]

Well you'd have to add the ShimError function that I posted above, and then add an import for each class that extends Error.

[DanielRosenwasser]

Actually, I wonder if you could get away with this, which doesn't need the __extends helper function:

export interface RxErrorStatic {
    new (message?: string): RxError;
}
export interface RxError extends Error {}

export const RxError: RxErrorStatic = function RxError(this: Error, message: string) {
    const err = Error.call(this, message);
    this.message = message;
    this.stack = err.stack;
    return err;
} as any;

RxError.prototype = Object.create(Error.prototype, {
    constructor: {
        value: RxError,
        enumerable: false,
        writable: true,
        configurable: true,
    },
});

[tetsuharuohzeki]

@kwonoj

What should I do about this? I feel the current patch is not different from @DanielRosenwasser proposed one.

[benlesh]

We solved this issue a while back, but I think you were involved with that. Either way, thanks for the contribution, @saneyuki. Closing this one for now.

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.