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

Deprecate the partition operator #3797

Closed
benlesh opened this issue Jun 5, 2018 · 23 comments
Closed

Deprecate the partition operator #3797

benlesh opened this issue Jun 5, 2018 · 23 comments

Comments

@benlesh
Copy link
Member

benlesh commented Jun 5, 2018

I think we should deprecate the partition operator and remove it for v7.

Reasons:

  1. Not really an operator: partition isn't really an "operator" in that it returns [Observable<T>, Observable<T>] rather than Observable<T>. This means it doesn't compose via pipe like the others.
  2. Easy to replace with filter: partition is easily replaced with the much more widely known filter operator. As partition is effectively the same thing as: const partition = (predicate) => [source.pipe(filter(predicate)), source.pipe(filter((x, i) => !predicate(x, i)))]
  3. Rarely used: It's little used, by any code survey I've taken (within thousands of lines of code that I know use RxJS)
@benlesh benlesh added type: discussion AGENDA ITEM Flagged for discussion at core team meetings labels Jun 5, 2018
@kwonoj
Copy link
Member

kwonoj commented Jun 5, 2018

I actually have quite lot of frequent usage around partition in my codebases. Rationale for partition is usually I want to have 2 streams at once instead of applying filter twice, i.e simple example like

[ onlineObservable, offlineObservable ] = partition((x) => x.isOnline));. Though this could be achieved as suggested snippet via filter.

@cartant
Copy link
Collaborator

cartant commented Jun 5, 2018

I think I've used it only once. However, I also think I'd be more likely to use it it if were to be a static function.

@kwonoj
Copy link
Member

kwonoj commented Jun 5, 2018

Having it as static makes sense to me too.

@cartant
Copy link
Collaborator

cartant commented Jun 6, 2018

I was thinking about this, this morning. I'd like to keep partition, but make it a static function - as suggested, by Ben, here - and also extend it so that more than one predicate can be passed. Something like this.

@benlesh
Copy link
Member Author

benlesh commented Jun 6, 2018

I did put together a static version of it in experimental, which you can see here... but that's what spurred me to think "this is so simple, and so little used, I'm not sure it's worth it"

@abdel-ships-it
Copy link

I do actually use it quite often, but destructuring two filtered sources actually does the exact same.

@benlesh benlesh removed the AGENDA ITEM Flagged for discussion at core team meetings label Jun 7, 2018
@abdel-ships-it
Copy link

@benlesh I think in codebases where its used quite often, it would be nice to have so you are not constantly repeating yourself.

@wmaurer
Copy link

wmaurer commented Jun 14, 2018

Now that I know that partition exists, I could see nice use cases for it. Please do make it static operator.

@bdadsetan
Copy link

@benlesh I understand that partition is rather simple, and I understand it might not be used as often as filter or map, however, it does clean up the code and express intent quite well.

#3807 would move us away from the lodash terminology known by many developers. I still believe that it should be called partition, and if it does not get the special treatment that publish also needs (as publish also does not return an Observable), then I would also argue for your initial idea of making it a static.

@benlesh
Copy link
Member Author

benlesh commented Jun 21, 2018

Now that I know that partition exists, I could see nice use cases for it. Please do make it static operator.

That's just the thing... it's not an operator as-is. It returns (source: Observable<T>) => [Observable<T>, Observable<T>], not (source: Observable<T>) => Observable<T>, so it doesn't compose via pipe. 🤷‍♂️

It's going to have to change one way or another. It's either:

  1. have a function : partition(source: Observable<T>, fn: (value: T, index: number) => boolean): [Observable<T>, Observable<T>
  2. or create a totally new operator: partitionThing(fn: (value: T, index: number) => boolean): Observable<[Observable<T>, Observable<T>]>
  3. or both?

@bdadsetan
Copy link

Should this discussion maybe wait until the discussion [1] about publish(), refCount() and so on are sorted, as they are probably also not composable by pipe? The partition() issue should probably not drive the design decision.

[1] #3833

@abdel-ships-it
Copy link

@benlesh I would go with the second one, as its quite similar to how the original partition worked. I think the first one is nice too, but having two different approaches that effectively solve the same problem can in my opinion be confusing.

@ivan7237d
Copy link

I recently suggested a simpler grouping operator (#3922) and maybe that operator would cover the partition's use-case well enough for partition to be deprecated.

@mattiLeBlanc
Copy link

I actually wanted to use partition in my NGRX Effects, I want to separate an httpEvent stream that sends event.type 0 to 4 and I want to throttle the type 3 events and also handle the type 4 (success).
I thought partition was ideal but I don't think I can throttle and merge 2 streams back into one. So when do you use partition?

@spearmootz
Copy link

I was using partition before and it worked great. i recently migrated to rxjs 6 and had a really weird bug when i switched to using two separate filter operators to achieve the result. essentially both filters received true even though the condition function were opposite.

so the issue happened because one of the filters receiving and object and resolving to true, it then ran a side effect function and because the value received was an object when the second filter ran the side effect function had mutated the original value and this second filter also resolved to true. this wouldnt have happened with partition.

as far as the two options above, i like 1

@maartentibau
Copy link

#3797 (comment)

I personally also would go for option 2

@martinsik
Copy link
Contributor

I agree partition() isn't very useful. I've answered around 750 questions on StackOverflow tagged as "rxjs" and I think I've never seen a good use-case for partition().

@baamenabar
Copy link

I actually wanted to use partition in my NGRX Effects, I want to separate an httpEvent stream that sends event.type 0 to 4 and I want to throttle the type 3 events and also handle the type 4 (success).
I thought partition was ideal but I don't think I can throttle and merge 2 streams back into one. So when do you use partition?

I have a simmilar use case. Handling a stream with diferent outcomes, or different error responses from an http connections is a great use case.

I suspect many people have tried to use partition, but given that it's not pipeable they give up on it and end up using filter or something else. I throw errors and filter them, but dislike throwing errors when the flow is actually a possible expected event in the stream.

@AodhanHayter
Copy link

I actually wanted to use partition in my NGRX Effects, I want to separate an httpEvent stream that sends event.type 0 to 4 and I want to throttle the type 3 events and also handle the type 4 (success).
I thought partition was ideal but I don't think I can throttle and merge 2 streams back into one. So when do you use partition?

I have a simmilar use case. Handling a stream with diferent outcomes, or different error responses from an http connections is a great use case.

I suspect many people have tried to use partition, but given that it's not pipeable they give up on it and end up using filter or something else. I throw errors and filter them, but dislike throwing errors when the flow is actually a possible expected event in the stream.

I'm one of those folks who was trying to use partition, then slowly discovering that it is not pipeable. In fact it's also on an httpEvent stream, would be very useful, but just awkward with its current implementation.

@spearmootz
Copy link

spearmootz commented Jan 18, 2019 via email

@BioPhoton
Copy link
Contributor

@benlesh
As partition operator is deprecated: https://rxjs.dev/api/operators/partition
and implemented as static: https://rxjs.dev/api/index/function/partition
we can close this issue

@doggy8088
Copy link
Contributor

@BioPhoton The partition looks like not exist in some RxJS 6 versions.

image

@cartant
Copy link
Collaborator

cartant commented May 1, 2021

Closing this because partition is deprecated and there is another issue - #3807 - that relates to its (possible) replacement.

@cartant cartant closed this as completed May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests