merge(with:) breaks RxSwift's merge()

[akolov]

Hi, I've stumbled upon an issue where importing RxSwiftExt breaks behavior of Observable.of(...).merge().

Given the following sample code:

Observable.of(a, b)
  .merge()
  .debug("+++ TEST")
  .subscribe(onNext: { x in
    print("BOO \(x)")
  })
  .disposed(by: disposeBag) // disposeBag is declared somewhere else and should retain the observable

  a.onNext("a")
  b.onNext("b")

Now, depending on whether I import RxSwiftExt or not produces drastically different result.

First, output without RxSwiftExt, everything works as expected:

+++ TEST -> Event next(a)
BOO a
+++ TEST -> Event next(b)
BOO b

Next, if I import RxSwiftExt, result is something completely unexpected. I get event completed and whole thing disposed at the end:

+++ TEST -> subscribed
+++ TEST -> Event next(RxSwift.PublishSubject<Swift.String>)
BOO RxSwift.PublishSubject<Swift.String>
+++ TEST -> Event next(RxSwift.PublishSubject<Swift.String>)
BOO RxSwift.PublishSubject<Swift.String>
+++ TEST -> Event completed
+++ TEST -> isDisposed

In fact, if I step into merge() when RxSwiftExt is imported, it actually goes inside merge(with:) method.

I can reliably reproduce this with RxSwift 5.0.1 and RxSwiftExt 5.1.1, both built with Carthage using Xcode 10 (10G8) and Xcode 11 GM 2 (11A491c).

Is this expected behavior or am I doing something wrong here?

I'm attaching a sample project I used to pinpoint the issue, please run carthage bootstrap to install RxSwift and RxSwiftExt.

merge-issue.zip

[freak4pc]

Hey, this definitely doesn't seem like an issue with the framework as these two methods have entirely different signatures.

@jdisho could you take a look at his example perhaps?

[jdisho]

Thanks for pointing it out. I will have a look asap 😊

[cliss]

+1. My project breaks with 5.1.1 but works with 5.0.0. All of my build errors in 5.1.1 were due to issues with merge().

[freak4pc]

Hey @jdisho - can you let me know if you'll be able to take a look in the next day or two? No problem if not, but I want to know since otherwise I'll just carve out some time to look into it :)

[jdisho]

Sorry for the late reply and not finding the time this weekend. Definitely, I will have a look tomorrow @freak4pc 😊

_{Sent with GitHawk}

[jdisho]

I see the problem... 🤔

[jdisho]

We can either rename to mergeWith(_ others:) or, convert the others to an array rather than having it as a sequence.

Any other suggestion?

[freak4pc]

the former isn't great because we already have zip(with:)
Would converting to an array work here ? Seems reasonable to me.

[cliss]

+1 to the idea of using an array. 😇

[freak4pc]

Cool, @cliss - any way for you to confirm that changing the method to use an array would solve the ambiguity for your code base ? I'll try taking a look myself tomorrow or so and push a PR.

[cliss]

Point me to a branch and I'm happy to give it a shot!

[freak4pc]

Hey @cliss - it's on the mergeWithArray branch :)
Thank you!

[akolov]

It definitely solves issues I was having with merge.

[freak4pc]

@cliss - would appreciate you letting me know as well before I'm cutting a new release :) thanks @akolov !

[cliss]

Crap, I'm so sorry; just saw this. Will test first thing tomorrow morning.

My utmost apologies!

[cliss]

@freak4pc Concur with @akolov; the mergeWithArray branch fixed my build. 🎉

Apologies, again, for the delay!

[freak4pc]

Merged it, will be out in a new release by next week most likely :)

[cliss]

@freak4pc bump 😇

[freak4pc]

Oh yikes! Thought I took care of this since I merged it 🤦‍♂
I just put a reminder to cut a release tomorrow. Thanks for the reminder! @cliss

[freak4pc]

@cliss and others - 5.2.0 was just released 🎉

[cliss]

Thanks @freak4pc! 🍻