ReactiveX / rxjs

A reactive programming library for JavaScript
https://rxjs.dev
Apache License 2.0
30.74k stars 3k forks source link

Bump up typescript compiler? #2582

Closed kwonoj closed 6 years ago

kwonoj commented 7 years ago

We're currently staying bit behind version of latest TypeScript compiler for some of compatibility reasons. now ng4.1 (http://angularjs.blogspot.com/2017/04/angular-410-now-available.html) is fully compatible with 2.3, our biggest concern seems resolve and maybe we can bump up compiler to use some of latest language features?

kwonoj commented 7 years ago

/cc @david-driscoll for visibility as well.

david-driscoll commented 7 years ago

I say lets do it. We can bump the compiler and then turn on strict: true as a separate PR.

cc @benlesh

kwonoj commented 7 years ago

afaik only currently blocking issue is https://github.com/ReactiveX/rxjs/pull/2217 .

tetsuharuohzeki commented 7 years ago

From rxjs-core-notes/2017-01/january-30.md:

Punt and work with ng2 team towards the same solution, since they have the problem too.

By this decision, we need to sort with angular.

From their code, I seem Angular have some pattern to construct an error:

  1. https://github.com/angular/angular/blob/master/packages/router/src/events.ts#L71-L84
    • I don't know about angular framework well, however, I think we cannot follow this approach because it would be a compat problem with IE11.
  2. https://github.com/angular/angular/blob/master/packages/router/src/events.ts#L71-L84
    • This pattern don't drive from Error object.
  3. https://github.com/angular/angular/blob/bebedfed24d6fbfa492e97f071e1d1b41e411280/packages/core/src/di/reflective_errors.ts#L39-L58

    • This pattern export a interface (InjectionError) and export a factory function which returns a new InjectionError.

    2217's approach is different from them.

I feel pattern-3 would reasonable way for us.

It does not have a complex exporting symbols like the approach I proposed in #2217. It make sense. The drawback point of pattern-3 is that we cannot create the error with new syntax. However, I don't feel it's serious problem because our user would not create rxjs' internal error except for testing.


@kwonoj @david-driscoll @benlesh

How about this?

david-driscoll commented 7 years ago

Option 3 seems reasonable to me.

tetsuharuohzeki commented 7 years ago

The drawback point of pattern-3 is that we cannot create the error with new syntax. However, I don't feel it's serious problem because our user would not create rxjs' internal error except for testing.

Ugh, I missed more drawback points.

If we accept this pattern, we cannot check a type of error with using e instanceof DerivedError. Even if we call Error.call(this) in the factory function, we cannot use its factory as the constructor for DerivedError in such case.

So we must define&expose a type guard functions like isDerivedError to check a type of error, and add a tag information (Symbol or magic string value) to DerivedError.

trxcllnt commented 7 years ago

fwiw, I found this pattern for defeating TypeScript's error-extending behavior. Not sure about browser compatibility, but it works in the TS playground (and also preserves the stack from inner errors, which we don't have today):

class UnsubscriptionError {
    public stack?: string;
    public message: string;
    public name: string = 'UnsubscriptionError';
    constructor(public errors?: any[]) {

        let msgErr = Error.call(this, !errors || !errors.length ? '' : `${
            errors.length} errors occurred during unsubscription: \n${
            errors.map((err, i) => `\t${i + 1}) ${err.toString()}`).join('\n')}`);

        let stackErr = Error.call(this, !errors || !errors.length ? '' : `${
            errors.length} errors occurred during unsubscription: \n${
            errors
                .map((err, i) => `\t${i + 1}) ` + (err.stack || '').replace('\n', '\n\t'))
                .join('\n')
            }`
        );

        msgErr.name = stackErr.name = this.name;

        if ((<any>Error).captureStackTrace) {
            (<any>Error).captureStackTrace(stackErr, UnsubscriptionError);
        }

        this.stack = stackErr.stack;
        this.message = msgErr.message;
        return this;
    }
}

UnsubscriptionError.prototype = Object.create(Error.prototype);

function flattenUnsubscriptionErrors(errors: any[]) {
    return errors.reduce((errs, err) => errs.concat((err instanceof UnsubscriptionError) ? err.errors : err), []);
}

try {
    try {
        throw new Error('foo');
    } catch (e) {
        throw new UnsubscriptionError([e]);
    }
} catch (e) {

    /* `true` */
    console.log(e instanceof UnsubscriptionError);

    /* Object `UnsubscriptionError` */
    console.log(e); 

    /*
    UnsubscriptionError: 1 errors occurred during unsubscription: 
        1) Error: foo
    */
    console.log(e.toString());

    /* 
    UnsubscriptionError: 1 errors occurred during unsubscription: 
        1) Error: foo
            at <anonymous>:26:15
            at HTMLButtonElement.excuteButton.onclick (https://www.typescriptlang.org/play/playground.js:242:39)
        at <anonymous>:29:15
        at HTMLButtonElement.excuteButton.onclick (https://www.typescriptlang.org/play/playground.js:242:39)
    */
    console.log(e.stack);
}
kwonoj commented 6 years ago

Now master runs latest.

lock[bot] commented 6 years ago

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.