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(typings): allow bufferCreationInterval null for bufferTime #3734

Merged
merged 2 commits into from
May 25, 2018

Conversation

mattiash
Copy link
Contributor

Description:

The tests for bufferTime verifies that it is possible to set
bufferCreationInterval to null, but the typings did not allow it
when strictNullChecks were set to true.

Related issue (if exists):

Fixes #3728

The tests for bufferTime verifies that it is possible to set
bufferCreationInterval to null, but the typings did not allow it
when strictNullChecks were set to true.

Fixes ReactiveX#3728
@coveralls
Copy link

coveralls commented May 23, 2018

Coverage Status

Coverage decreased (-0.2%) to 96.654% when pulling ada645c on mattiash:buffertime-typings into c05a945 on ReactiveX:master.

@cartant
Copy link
Collaborator

cartant commented May 23, 2018

My two cents: Is this consistent with other parts of the API? Are there other places where null is used rather than undefined? I can't recall an API method to which null is passed to indicate that a parameter is not specified in a call; I can only recall those to which undefined is passed.

For example, undefined is used to to subscribe. This is okay:

of(1).subscribe(undefined);

Whereas this is not:

of(1).subscribe(null); // [ts] Argument of type 'null' is not assignable to parameter of type '((value: number) => void) | undefined'.

For consistency, I think it should be undefined, rather than null. Or all of the typings should be changed to accept either null or undefined for parameters that are to be ignored.

@mattiash
Copy link
Contributor Author

The tests pass in null for bufferCreationInterval: https://github.com/ReactiveX/rxjs/blob/master/spec/operators/bufferTime-spec.ts#L24

The code sets bufferCreationInterval to null if the parameter is not included:
https://github.com/ReactiveX/rxjs/blob/master/src/internal/operators/bufferTime.ts#L67

and it later checks if it is null:
https://github.com/ReactiveX/rxjs/blob/master/src/internal/operators/bufferTime.ts#L129

Changing it to undefined is a much bigger change, but I have nothing against it.

@cartant
Copy link
Collaborator

cartant commented May 23, 2018

Changing it to undefined is a much bigger change ...

Yeah, you are right. It would be a bigger change and smaller changes are always more appealing.

Regarding the check against null, the check uses == which will return true for either undefined or null. That's the mechanism used throughout the Angular codebase, IIRC.

Anyway, others can decide, I just thought I'd point out the consistency issue.

Thanks for the PR, BTW. 👍

@benlesh
Copy link
Member

benlesh commented May 24, 2018

Yeah, this should probably be number|null|undefined. (honestly we have other places that only accept undefined that should probably accept null as well. I don't think there are any major penalties for that.

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

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

add undefined to the signature as well, please.

@benlesh benlesh merged commit 0bda9cd into ReactiveX:master May 25, 2018
@benlesh
Copy link
Member

benlesh commented May 25, 2018

Thanks, @mattiash!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 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