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: takeWhile Boolean constructor types #6633

Merged

Conversation

josepot
Copy link
Contributor

@josepot josepot commented Oct 7, 2021

I think that the typings for the Boolean constructor of takeWhile are off... For instance, I think that the following dtslint test:

const c = of(
  false as const,
  0 as const,
  "hi" as const,
  -0 as const,
  0n as const,
  "" as const,
  null,
  undefined
).pipe(takeWhile(Boolean)); // $ExpectType Observable<false | "" | 0 | 0n | "hi" | null | undefined>

should expect this, instead:

const c = of(
  false as const,
  0 as const,
  "hi" as const,
  -0 as const,
  0n as const,
  "" as const,
  null,
  undefined
).pipe(takeWhile(Boolean)); // $ExpectType Observable<"hi">

The goal of this PR is to make the Boolean constructor typings of takeWhile (when the inclusive flag is set to false) consistent with the typings of filter.

@kwonoj
Copy link
Member

kwonoj commented Oct 7, 2021

Not sure if I understood correctly, but looks like current signature behaves correct? Takewhile doesn't change what source emits but only limits when to stop emit. Would you mind explain bit as I may miss something?

@josepot
Copy link
Contributor Author

josepot commented Oct 7, 2021

Not sure if I understood correctly, but looks like current signature behaves correct? Takewhile doesn't change what source emits but only limits when to stop emit. Would you mind explain bit as I may miss something?

If the inclusive flag is set to false (which it's its default value) then if the takeWhile operator is used with the Boolean constructor the resulting observable won't emit falsy values. For instance:

of('foo', 'bar', null).pipe(
  takeWhile(Boolean)
)

will only emit strings, it would never emit null.

That's why I think that it makes sense to change the typings of the Boolean constructor (when the inclusive flag is set to false), so that the typings of the resulting Observable exclude the falsy values, like filter does.

@josepot josepot force-pushed the fix/takewhile-boolean-constructor branch from e030311 to 1dbe556 Compare October 7, 2021 21:27
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.

@benlesh benlesh added 7.x Issues and PRs for version 6.x 8.x Issues and PRs for version 8.x labels Dec 6, 2021
@benlesh benlesh merged commit 081ca2b into ReactiveX:master Dec 6, 2021
@benlesh
Copy link
Member

benlesh commented Dec 6, 2021

Thank you, @josepot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
7.x Issues and PRs for version 6.x 8.x Issues and PRs for version 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants