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(endWith): wrap args - they are not observables - in of before concatenating #4735

Merged
merged 3 commits into from May 2, 2019

Conversation

@cartant
Copy link
Collaborator

commented Apr 26, 2019

Description:

This PR spreads the arguments passed to endWith into a call to of which is then passed to `concat.

The current implementation spreads the args directly into concat which is incorrect. The tests pass because strings are iterable and are therefore valid observable inputs. And the strings in the tests each contain only a single character. The test added in this PR uses numbers for values, and their use effects an error from the current, incorrect endWith implementation.

Related issue (if exists): #4731

@cartant cartant requested a review from benlesh Apr 26, 2019
@coveralls

This comment has been minimized.

Copy link

commented Apr 26, 2019

Pull Request Test Coverage Report for Build 8438

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.0005%) to 96.857%

Totals Coverage Status
Change from base Build 8414: 0.0005%
Covered Lines: 5793
Relevant Lines: 5981

💛 - Coveralls
@cartant cartant referenced this pull request May 1, 2019
@benlesh
benlesh approved these changes May 2, 2019
@benlesh benlesh merged commit 986be2f into ReactiveX:master May 2, 2019
3 checks passed
3 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.0005%) to 96.857%
Details
@benlesh

This comment has been minimized.

Copy link
Member

commented May 2, 2019

Oh wow! What a cool bug that made it through all of those tests! Since 'x' is an iterable, concat(obs$, 'x') is the same thing as concat(obs$, of('x')). 🤣

@AlexAegis

This comment has been minimized.

Copy link

commented May 12, 2019

In v6.5.1 this worked:

endWith(of(undefined))

In v6.5.2 this works, just as the changelog says:

endWith(undefined)

But right now in v6.5.2 endWith(undefined) is marked as deprecaded. Am I doing something wrong or is this a bug?

@cartant

This comment has been minimized.

Copy link
Collaborator Author

commented May 12, 2019

@alex-wilmer It's matching the signature with the scheduler in it. Most likely because you do not have strictNullChecks enabled. See this comment and this comment.

As mentioned in the latter comment, IMO, the deprecation in the TSDoc should not be removed to avoid the match in situations in which strictNullChecks are disabled. Override the lint failure using the mechanism mentioned in that comment.

IMO, this is a documentation issue.

BioPhoton added a commit to BioPhoton/rxjs that referenced this pull request May 15, 2019
…catenating (ReactiveX#4735)

* test(endWith): add failing test

* fix(endWith): wrap args in of before concat

* chore(endWith): remove any assertion
@lock lock bot locked as resolved and limited conversation to collaborators Jun 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants
You can’t perform that action at this time.