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 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 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 Oct 7, 2021
cartant
cartant approved these changes Oct 9, 2021
Copy link
Collaborator

@cartant cartant left a comment

LGTM. Thanks.

@benlesh benlesh added 7.x 8.x labels Dec 6, 2021
@benlesh benlesh merged commit 081ca2b into ReactiveX:master Dec 6, 2021
5 checks passed
@benlesh
Copy link
Member

@benlesh 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 8.x
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants