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

refactor: refactor throwError operator to use factory function in unit tests and .ts file #6261

Merged

Conversation

nitinmalave
Copy link
Contributor

Description:

  • As per docs, throwError accepts factory functions to emit an error and no longer able to accept an error directly. That's why i went ahead and made changes in all unit tests wherever throwError is getting used to accept factory function.

Related issue (if exists):
Noop

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

I'm okay with the wholesale replacement in the implementation tests, but I think the dtslint tests - spec-dtslint/observables/throwError-spec.ts - need to include both. So, rather than replace the calls in the dtslint tests, copy them and then replace them.

Also, this PR seems to have some conflicts with what's currently in master.

@nitinmalave
Copy link
Contributor Author

nitinmalave commented Apr 26, 2021

Hi @cartant, i am little confuse here. Can you please help me with what i understood is correct or not?
As per your suggestion,

  • changes which needs to be done are in 'dtslint/observables/throwError-spec.ts' file only, right?
  • 'dtslint/observables/throwError-spec.ts' should have 2 versions of throwError and is should look like this :

`it('should accept any type and return never observable', () => {
const a = throwError(1); // $ExpectType Observable
const a1 = throwError(() => (1)); // $ExpectType Observable
const b = throwError('a'); // $ExpectType Observable
const b1 = throwError(() => ('a')); // $ExpectType Observable
const c = throwError({a: 1}); // $ExpectType Observable
const c1 = throwError(() => ({ a: 1 })); // $ExpectType Observable
const d = throwError(() => ({ a: 2 })); // $ExpectType Observable
});

it('should support scheduler', () => {
const a = throwError(1, animationFrameScheduler); // $ExpectType Observable
const a1 = throwError(() => (1, animationFrameScheduler)); // $ExpectType Observable
});`

Please correct me if i am wrong.

@cartant
Copy link
Collaborator

cartant commented Apr 26, 2021

I'd create separate it functions to make it clearer:

it('should support an error value and a scheduler', () => {
  const a = throwError(1, animationFrameScheduler); // $ExpectType Observable<never>
});`

it('should support a factory and a scheduler', () => {
  const a = throwError(() => 1, animationFrameScheduler); // $ExpectType Observable<never>
});`

Also, you have the parentheses in the wrong place for the last test and that's what's failing in CI.

@nitinmalave nitinmalave requested a review from cartant April 26, 2021 09:43
@nitinmalave
Copy link
Contributor Author

Got it. Thanks @cartant for quick support. I have committed this changes. Please take a look at it.

Copy link
Collaborator

@cartant cartant left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@cartant cartant merged commit 6ef2725 into ReactiveX:master Apr 27, 2021
@nitinmalave nitinmalave deleted the throwError-updated-factory-function branch May 28, 2021 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants