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(expand): accept scheduler parameter #858

Closed
wants to merge 1 commit into from

Conversation

luisgabriel
Copy link
Contributor

Also moves the handling of the default value for optional parameters to
the expand function instead of the operator's ctor.

Closes #841.

@luisgabriel
Copy link
Contributor Author

@kwonoj @Blesh could you guys take a look if I'm going in the right direction here? ;)

@@ -22,12 +26,25 @@ export class ExpandSubscriber<T, R> extends OuterSubscriber<T, R> {
private active: number = 0;
private hasCompleted: boolean = false;
private buffer: any[];
private _concurrent: number = Number.POSITIVE_INFINITY;
Copy link
Member

Choose a reason for hiding this comment

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

is it necessarily required to have 2 fields, _concurrent and concurrent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is not. I'll fix it.

Also moves the handling of the default value for optional parameters to
the expand function instead of the operator's ctor.

Closes ReactiveX#841.
@luisgabriel luisgabriel changed the title [WIP] fix(expand): accept scheduler parameter fix(expand): accept scheduler parameter Dec 4, 2015
@luisgabriel
Copy link
Contributor Author

Just updated this PR addressing the previous comments.

Still missing some test cases using a custom scheduler. Suggestion are very welcome. ;)

@@ -46,19 +53,26 @@ export class ExpandSubscriber<T, R> extends OuterSubscriber<T, R> {
let result = tryCatch(this.project)(value, index);
if (result === errorObject) {
destination.error(result.e);
} else if (!this.scheduler) {
this.subscribeToProjection(result, value, index);
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we've lost the scalar optimization for this branch of code. if (result._isScalar) { ...etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was moved to subscribeToProjection().

Copy link
Member

Choose a reason for hiding this comment

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

I see. Thanks.

@benlesh
Copy link
Member

benlesh commented Dec 8, 2015

Merged with 79e9084. Thanks @luisgabriel!

@benlesh benlesh closed this Dec 8, 2015
@luisgabriel luisgabriel deleted the expand-scheduler branch December 8, 2015 00:40
@lock lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019
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