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.

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

fix(ajax): RxJS v6 TimeoutError is missing name property #3656

Merged
merged 2 commits into from
May 9, 2018

Conversation

bbonnet
Copy link
Contributor

@bbonnet bbonnet commented May 7, 2018

Description:
Update the name property of the AjaxTimeoutError to be "AjaxTimeoutError" instead of "AjaxError".

@felixfbecker mentioned the following in the original issue:

Why not use instanceof? instanceof breaks when multiple RxJS versions are in play, or even just the same version had to be installed multiple times because deduping wasn't possible. It asserts the implementation, err.name asserts an interface. All browser errors use it (e.g. AbortError).

This still will cause problems if the implementation name changes. Open to feedback for a more permanent solution.

Related issue (if exists):
#3602

@coveralls
Copy link

coveralls commented May 7, 2018

Coverage Status

Coverage increased (+0.001%) to 96.505% when pulling 44042d0 on bbonnet:fix-timeout-error-name into 7e4cef4 on ReactiveX:master.

@felixfbecker
Copy link
Contributor

Note this doesn't fix #3602, as that uses a different error.

@bbonnet
Copy link
Contributor Author

bbonnet commented May 7, 2018

Ah I see, that is my mistake. I will look into a better fix that will cover the TimeoutError as well

@bbonnet
Copy link
Contributor Author

bbonnet commented May 7, 2018

Updated to fix TimeoutError.

Also noticed that some of the tests that are implemented like the following are not hitting some of the expectations:. This test will still pass.

   result.subscribe(() => {
      throw new Error('this should not next');
    }, err => {
      // Never actually getting tested
      expect(err).to.be.an.instanceof(Rx.TimeoutError);
      expect(err.name).to.equal('BlahBlahBlah');
    }, () => {
      throw new Error('this should not complete');
    });

I updated as follows to get the assertions to actually hit. Seen this pattern elsewhere in the tests.

    let error;
    result.subscribe(() => {
      throw new Error('this should not next');
    }, err => {
      error = err;
    }, () => {
      throw new Error('this should not complete');
    });
    rxTestScheduler.flush();
    expect(error).to.be.an.instanceof(Rx.TimeoutError);
    expect(error.name).to.equal('TimeoutError');

this.name = 'TimeoutError';
(Object as any).setPrototypeOf(this, TimeoutError.prototype);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

did line endings change in this file?

@benlesh benlesh merged commit 940a121 into ReactiveX:master May 9, 2018
@benlesh
Copy link
Member

benlesh commented May 9, 2018

Thank you, @bbonnet!

@@ -8,7 +8,7 @@
export class TimeoutError extends Error {
constructor() {
super('Timeout has occurred');

this.name = 'TimeoutError';
Copy link
Contributor

Choose a reason for hiding this comment

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

it would be nice if this was declared as a property so it is a visible part of the API contract:

public readonly name: 'TimeoutError' = 'TimeoutError'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@felixfbecker Following up with that here: #3678

@lock lock bot locked as resolved and limited conversation to collaborators Jun 9, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants