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

👷‍♂️👷‍♀️RxSwift 5 Pre-Release discussion #1921

Closed
23 tasks done
freak4pc opened this issue Apr 8, 2019 · 67 comments
Closed
23 tasks done

👷‍♂️👷‍♀️RxSwift 5 Pre-Release discussion #1921

freak4pc opened this issue Apr 8, 2019 · 67 comments
Assignees
Milestone

Comments

@freak4pc
Copy link
Member

freak4pc commented Apr 8, 2019

Hey @kzaher & others -

Since RxSwift 5 is around the corner, I took the liberty of making a list of PRs/Issues that were delayed for RxSwift 5.

The goal here is to see if:

  • Any of them are easy enough to get into RxSwift 5; or,
  • Any are hard to add but are needed for RxSwift 5 (and worth delaying the release for).

Since this is a major version bump we should take some time to think about any breaking changes we might want to introduce.

This is what I gathered so far:

Generic Renames Checklist:

If there are any other PRs/Issues you feel should participate in this discussion, please add a comment below.

@freak4pc
Copy link
Member Author

freak4pc commented Apr 8, 2019

@kzaher the purpose is mainly to see which of these we want to do for RxSwift 5, as long as we're making breaking changes here.

@freak4pc freak4pc changed the title 👷‍♂️RxSwift 5 Pre-Release discussion 👷‍♂️👷‍♀️RxSwift 5 Pre-Release discussion Apr 8, 2019
@valerianb
Copy link

Maybe Variable can be definitely removed from the repo ? It has been in Deprecated for a while now.

@freak4pc
Copy link
Member Author

freak4pc commented Apr 8, 2019

@valerianb Great point. I don't think we should entirely remove it, but we probably should move everything from runtime deprecations to hard deprecation (e.g. @available(*, deprecated)). Thoughts?

@valerianb
Copy link

Oh I thought there was a similar mechanism already in place with DeprecationWarner. But yes if that's not considered enough to push users to transition away from it then we should start with @available(*, deprecated) so it can be dropped in a future release.

@kzaher
Copy link
Member

kzaher commented Apr 8, 2019

Hi @freak4pc, I think that these are small changes we can add.

doAfter* overloads to match RxJava. #1898
Move Relays to their own framework. #1501
Allow binding to multiple observers. #1702
combineLatest of an empty array completes immediately. #1879
Change toArray to return Single. #1543

If you can help with list on top, that would be great.

Decide how to deal with soft-deprecated things (Variable, global RxTest things)

I think we can deprecate Variable.

Using DispatchTimeInterval instead of TimeInterval for Scheduleers. #1472

I think we should also include this. This one is probably the biggest and requires intimate knowledge of the internals so I can take that one.

Complete once any Completable comples. #1674

I've responded to that thread, I think we can close this one. Not sure there is an action point on it. zip and merge are equivalent. We can add zip operator if you like, but we need to change the way zip operator works and not dispose subscriptions when no new element can be produced anymore.

@freak4pc
Copy link
Member Author

freak4pc commented Apr 8, 2019

I can take over the top list, no problem.

I’m also ok with closing the Completable one- we might be able to tackle this at a later release, I’d leave that to your discretion.

@hmlongco
Copy link

@freak4pc Would love to have the compactMap PR in 5.

@freak4pc
Copy link
Member Author

It's in the list (look at the end) ;-)

@hmlongco
Copy link

hmlongco commented Apr 12, 2019

Ummmm.... I could have sworn.... ;)

@kzaher
Copy link
Member

kzaher commented Apr 14, 2019

@freak4pc I've refactored schedulers. I don't think there is any more confusion with take and count.

@freak4pc
Copy link
Member Author

I mentioned on the other thread that I still think it's worth adding a label for one of them to differentiate. Types alone aren't enough in my personal opinion.

@DevAndArtist
Copy link
Contributor

@kzaher @freak4pc I opened a PR to fix resultSelector mistakes.

#1930

@kzaher
Copy link
Member

kzaher commented Apr 15, 2019

I think that the last blocker for 5.0 is #1799.

@freak4pc
Copy link
Member Author

We won't be able to release this for 5.0 because of the Xcode 10.2 Swift 5 issue I've mentioned. We should release without it.

Should we also leave the take() overloads as is? I still think it's worth adding a label to one of them at least.

@kzaher
Copy link
Member

kzaher commented Apr 15, 2019

This is swift prefix api. Which is similar to our take.

https://developer.apple.com/documentation/swift/array/1689487-prefix

It doesn't have label for count and we don't have a label for time operators.

I think that the problem was worse before when you could specify a constant which could mean two different overloads.

Right now it should be more clear. I'm personally fine with the current state. Adding a label to count would be against Swift prefix API, and if we add label to time version we would need to add that label everywhere for consistency. I'm not even sure what label would we use.

@freak4pc
Copy link
Member Author

Sounds good. I think this concludes the RxSwift 5 content we discussed unless you have any more thoughts.

@freak4pc
Copy link
Member Author

@kzaher Actually prefix has multiple overloads and anything aside from the "count" one has a label ..
image

@kzaher
Copy link
Member

kzaher commented Apr 15, 2019

Actually prefix has multiple overloads and anything aside from the "count" one has a label ..

@freak4pc I know, that's what I've posted that link.

Adding a label to count would be against Swift prefix API, and if we add label to time version we would need to add that label everywhere for consistency. I'm not even sure what label would we use.

@freak4pc
Copy link
Member Author

Ah, I misunderstood. I was also referring to labeling the duration overload, not the count one.

I though of either take(duration:), or take(for duration:). WDYT?

@kzaher
Copy link
Member

kzaher commented Apr 15, 2019

I think that take(for:) reads better, but for is a keyword and not really describing temporal component IMHO. Just Google take for and you'll get something like "take for a word" as a top hit.

You look at this as a decision for a single operator. I look at this as a decision to go through all temporal operators and decide new labels for each of them and deprecate the old ones also.

Without extremely convincing reason I would not want to do all of that.

@freak4pc
Copy link
Member Author

Don't really understand what you're referring to with stdlib, feel free to share with us.

The concrete examples are everywher, just look for O: ObservableConvertibleType or O: ObservableType.

@DevAndArtist
Copy link
Contributor

DevAndArtist commented Apr 27, 2019

I‘m currently only on my iPad, I‘ll check later. But in general I would prefer a stdlib like coding style, that makes things easier to read (that is ofcourse my opinion).

func foo<AnObservable>(
  _ observable: AnObservable
) -> Observable<AnObservable.Element> 
  where
  AnObservable: ObservableType
{
  ...
}

That ofcouse won‘t be rendered by the ASTPrinter like that when the user cmd + click on the API in Xcode.


That aside AnObservable would be fine by me to not to create any ambiguity that will force us or in worst case the users to use RxSwift.Observable explicitly.

Sent with GitHawk

@freak4pc
Copy link
Member Author

Yup, it's definitely a personal opinion. I don't mind either style but I actually find the style where the protocol of the constraint is inline more readable. Personal preference for sure :)

@kzaher
Copy link
Member

kzaher commented Apr 27, 2019

I've solved O.
Pushed changes to develop.

Is there anything else we want to do in the public interface?

@freak4pc
Copy link
Member Author

I think that takes care of only the public API - for the rest I already did the other APIs as well just to be thorough. Do you want to leave that for now? Or should we go with Source everywhere?

@kzaher
Copy link
Member

kzaher commented Apr 27, 2019

Maybe we should use Source to resolve ambiguities.

@freak4pc
Copy link
Member Author

@kzaher Rest of the cleanups should be in #1958

@DevAndArtist
Copy link
Contributor

I just realized one interesting fact. From SE the basic direction for generics UI goes into some Collection spelling for simple generic parameters. In that sense you could name the parameters SomeObservable which requires two more characters, but later on you could use them as some ObservableType if you do not need referencing the type directly to extract some other type information. But the choice is yours.

Sent with GitHawk

@freak4pc
Copy link
Member Author

If and when that SE would actually happen, we can have another discussion. Even though to me "Some" is superfluous.

@DevAndArtist
Copy link
Contributor

DevAndArtist commented Apr 27, 2019

Sure :) My point was that Some prefixed generic types or some P describe an opaque type, but the type is not known for the implementor of the method, as it‘s an input (parameter) type. Opaque type as the return type is the opposite because it‘s only known to the implementor but not the user, this is also known as reverse generics in SE. So if you need a simple type you can consider prefixing it with Some in case you‘d want it to become some MyType one day.

Sent with GitHawk

@kzaher
Copy link
Member

kzaher commented Apr 27, 2019

I think this should be everything for 5.0, or do we have something else?

@freak4pc
Copy link
Member Author

I can’t think of anything else, personally.

@freak4pc freak4pc reopened this Apr 27, 2019
@kzaher
Copy link
Member

kzaher commented Apr 28, 2019

I'm publishing 5.0, and everything went ok until ....

http://cocoapods.org/pods/RxRelay

Maintained by Wassim Seifeddine.

God damn it.

@DevAndArtist
Copy link
Contributor

@kzaher the repository is dead for 2 years and it's basically doing exactly the same we want to do.

@wassimseif can we find an agreement and reclaim the cocoapods spec namespace for RxRelay?

@freak4pc
Copy link
Member Author

Getting the privileges from @wassimseif would be optimal. Otherwise we can reach to the CocoaPods team and see if they'll be willing to help (I've seen several scenarios they were able to)

@kzaher
Copy link
Member

kzaher commented Apr 28, 2019

I've had to delete 5.0.0 of RxSwift because of this :(

@kzaher
Copy link
Member

kzaher commented Apr 28, 2019

I've sent an email to @wassimseif. I guess one thing is sure, this won't be released today probably :( Damn it.

@freak4pc
Copy link
Member Author

I also e-mailed him haha :)

@DevAndArtist
Copy link
Contributor

Any everyone pinged him here at least three times. 🤞

@freak4pc
Copy link
Member Author

haha :) true.

@kzaher
Copy link
Member

kzaher commented Apr 28, 2019

Well, we had some bad experiences with name camping. Somebody has https://github.com/RxSwift organization and didn't want to transfer it a while ago :) That's why we moved the project to ReactiveX :)

@DevAndArtist
Copy link
Contributor

The good thing about the repository is that it's dead and it's doing what we're doing here now. :)

@freak4pc
Copy link
Member Author

Actually think being part of ReactiveX is much nicer and more confident-inducing anyways :]

@freak4pc
Copy link
Member Author

Pod name ownership was transferred successfully, we're good to go @kzaher ! 🎉

@kzaher
Copy link
Member

kzaher commented Apr 29, 2019

@freak4pc I'll do it tonight, I need to change pod description because cocoapods were also complaining about that.

@freak4pc
Copy link
Member Author

Sounds good. Once that's done you might want to do pod trunk delete RxRelay 0.1.3 to get rid of the old project.

@kzaher
Copy link
Member

kzaher commented Apr 29, 2019

@freak4pc

This is done.

Sounds good. Once that's done you might want to do pod trunk delete RxRelay 0.1.3 to get rid of the old project.

We should never delete any tags.

@kzaher kzaher closed this as completed Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants