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

feat(join): add join operator #1371

Closed
wants to merge 2 commits into from
Closed

Conversation

luisgabriel
Copy link
Contributor

Closes #1341.

@kwonoj
Copy link
Member

kwonoj commented Feb 21, 2016

I've thought about bit if it's possible to make this without exposing outerIndex as public using innersubscription management looks like

interface Context<T1, T2> {
  observable: Observable<T1 | T2>;
  subscription: Subscription;
}

class join {
  private left: Context<T1, T2>;
  private right: Context<T1, T2>; 

  protected _next(value: Observable<T1 | T2>): void {
    (this.left) ? this.right = { observable: value, subscription: null } : this.left = { observable: value, subscription: null};
  }
}

rough proof of concept can be seen at kwonoj@1e06664, what do you think? since it's rough test code, still few topics I'd like to improve such as

  • not using map for managing values?
  • better duration completion handling?

and some others I couldn't think of.

@kwonoj
Copy link
Member

kwonoj commented Feb 21, 2016

+clarification : I'm not suggesting to update PR as comment. Just kicking off discussion. Not even sure if innersubscription management is efficiently perform compare to original PR.

*/
export function join<T1, T2, D1, D2, R>(right: Observable<T2>,
leftDurationSelector: (value: T1) => Observable<D1>,
rightDurationSelector: (value: T2) => Observable<D2>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type of the duration selector observables should be any, since the typing doesn't at all matter there. So we don't need D1 and D2, I don't think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. I'll remove D1 and D2.

@luisgabriel
Copy link
Contributor Author

@kwonoj I think I can improve the solution with some of your suggestions. ;) I'll update this PR as soon as I can.

Answering some specific parts:

rough proof of concept can be seen at kwonoj/rxjs@1e06664, what do you think? since it's rough test code

I was not very happy with exposing outerIndex as a public member (maybe a getter would be better?), but I think this is a useful information to have on notifyComplete() the same way it is available in notifyNext(). Is there a reason not having it on notifyComplete()?

not using map for managing values?

The items can be removed in any order depending on duration. The first item added can be the last to be removed, i.e. there is no relation between when the item is added to the list and when it is removed. We could use an array instead, but it'll require searching for the item. Map seems a more approriate solution.

better duration completion handling?

I tried to keep it as similar as possible to how RxJS 4 is handling completion to make sure it behaves similarly.

@kwonoj
Copy link
Member

kwonoj commented Feb 23, 2016

Is there a reason not having it on notifyComplete()?

I don't have clear visibility on those too. I think you may able to file an issue with compelling usecases. As similar, notifyNext didn't have innerSub as parameter and it's added recently, my poc branch utilized it. Personally, yet not sure about having idx in error / complete.

Map seems a more approriate solution.

In general I agree, that question was just came out of curiosity if it's possible to do so.

@benlesh
Copy link
Member

benlesh commented Mar 1, 2016

Looks like this will need to be rebased.

@benlesh
Copy link
Member

benlesh commented Mar 1, 2016

Also, I think I'd like to block this until #1364 is merged. After that you'll need to convert your tests to TypeScript. Cool?

@benlesh benlesh added the blocked label Mar 1, 2016
@luisgabriel
Copy link
Contributor Author

Sure. No problem.

@benlesh
Copy link
Member

benlesh commented Mar 2, 2016

Okay, this just needs rebased now, I think.

@luisgabriel
Copy link
Contributor Author

I'll update it later today.

@luisgabriel
Copy link
Contributor Author

Just updated the PR.

Changes:

  • Remove D1 and D2 type parameter
  • Expose outerIndex of InnerSubscriber as a getter instead of make it public
  • Add a Context class to encapsulate the state related to each observable that is being joined
  • Move durationSelector subscription to a separate function (try/catch optimization)
  • Completion handling is simpler now due to the use of Context
  • Change join declaration on Observable and CoreOperators to use JoinSignature
  • Move unit tests to TypeScript

@luisgabriel
Copy link
Contributor Author

@kwonoj I'm still using the index/id approach but I think it's simpler now with Context.

@kwonoj
Copy link
Member

kwonoj commented Mar 3, 2016

I'm still in quite favor of not exposing outerindex completely, but that's purely my own :) change looks ok to me. Once @Blesh approves, I'll check in this.

@luisgabriel
Copy link
Contributor Author

@kwonoj I kept using the index/id approach for two reasons:

  1. I don't like very much having a Map where the key is a reference type
  2. Without something that can identify the observable that is completing, we have to "guess" in which Map the Subscription is trying to delete it from the left first and if it's not there, deleting from the right.

The first is not a strong argument as it's a matter of taste/style. But the second have performance implications. If the subscription is in the right Map, it'll walk through the whole left Map looking for a subscription that is not there.

That said, I have no problem in changing the implementation if you guys prefer not to expose the outerIndex from InnerSubscription.

@kwonoj
Copy link
Member

kwonoj commented Mar 5, 2016

@luisgabriel Yes I understand, reason I mentioned it was 'my favor', and not insist to change it :)

@benlesh
Copy link
Member

benlesh commented Mar 9, 2016

This is a pretty in-depth implementation. It's going to take more than a while to review. Can you please add perf tests? It's important that we make sure this hits the perf numbers well.

@luisgabriel
Copy link
Contributor Author

@Blesh sure. No problem.

@benlesh
Copy link
Member

benlesh commented Mar 17, 2016

Hey, @luisgabriel ... sorry I haven't gotten back to this one yet. Hey, @trxcllnt, can you give this a review if you have the time?

@luisgabriel
Copy link
Contributor Author

@Blesh No problem. I didn't have time to add the perf tests yet. I'll add them as soon as a can.

@benlesh
Copy link
Member

benlesh commented Mar 30, 2016

Sorry, @luisgabriel, this is going to need rebased again. While this PR looks fine to me, the major hold up is I'm looking for someone that actually uses join a lot to review the PR. @mattpodwysocki, can you lend a hand? Also pinging @trxcllnt again.

@luisgabriel
Copy link
Contributor Author

@Blesh I'll take some time next weekend to work on this. I have to rebase it and change the interface to accept Subscribable instead of Observable.

@benlesh
Copy link
Member

benlesh commented Apr 12, 2016

Ping @luisgabriel ... just checking in.

@luisgabriel
Copy link
Contributor Author

@Blesh I'm having trouble with the tests after rebasing. I'll try to fix it.

@luisgabriel
Copy link
Contributor Author

@Blesh Sorry for the long delay. I'm no sure when I can work on this. So I'm going to close this for now. As soon as I can, I'll revisit this patch and submit a new PR.

@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants