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(startWith): remove deprecated scheduler signature. #7166

Merged
merged 5 commits into from May 25, 2023

Conversation

benlesh
Copy link
Member

@benlesh benlesh commented Jan 24, 2023

  • Removes deprecated scheduler signature
  • Improves performance slightly
  • Moves subscribeToArray closer to usage.

@benlesh benlesh force-pushed the startWith-remove-scheduler-arg branch from 7098f11 to 7b2a806 Compare January 24, 2023 03:43
Comment on lines 6 to 7
// Devs are more likely to pass null or undefined than they are a scheduler
// without accompanying values. To make things easier for (naughty) devs who
Copy link
Contributor

@demensky demensky Jan 24, 2023

Choose a reason for hiding this comment

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

After the scheduler was removed, these comments became useless. Maybe we should remove them?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are signatures with null and undefined still relevant?

Copy link
Member

Choose a reason for hiding this comment

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

I think they aren't relevant anymore.

Comment on lines 88 to 90
for (let i = 0; i < length && !subscriber.closed; i++) {
subscriber.next(array[i]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we exit the function as soon as subscriber.closed becomes true. Does it make sense to do something similar here?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is covered in the for loop. This part: && !subscriber.closed should ensure that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's just that in example, when subscriber.closed becomes true, then subscriber.complete() is not called, but in subscribeToArray it is called.

Copy link
Member

Choose a reason for hiding this comment

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

If subscriber.complete() gets called, but subscription is already closed, it won't make any difference. Subscriber will not get complete notification multiple times, so this is OK I think.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I know that complete won't be notified several times. I just wanted to ask and suggest doing something like this:

Suggested change
for (let i = 0; i < length && !subscriber.closed; i++) {
subscriber.next(array[i]);
}
for (let i = 0; i < length; i++) {
if (subscriber.closed) {
return;
}
subscriber.next(array[i]);
}

Nevermind, it's just my curiosity.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < length && !subscriber.closed; i++) {
subscriber.next(array[i]);
}
for (let i = 0; i < length; i++) {
if (subscriber.closed) {
return;
}
subscriber.next(array[i]);
}

I double-checked and made sure that if you do not return inside the loop, then onStoppedNotification will be called.

config.onStoppedNotification = (notification) => {
  console.log('onStoppedNotification', notification);
};

EMPTY.pipe(startWith(1, 2, 3), take(2)).subscribe();

Copy link
Member Author

Choose a reason for hiding this comment

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

It is mildly more efficient, I suppose. A conditional followed by a return, rather than a conditional followed by a function call, a conditional and a return. It's not likely to make any performance difference though, I'd guess. Even at high-speed.

Copy link
Contributor

Choose a reason for hiding this comment

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

We are talking about a false positive onStoppedNotification call.

Copy link
Contributor

Choose a reason for hiding this comment

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

The difference is that return prevents subscriber.complete().

const expected = 's--a--b--- ';
const values = { s: 's', a: 'a', b: 'b' };

const result = e1.pipe(startWith('s', testScheduler));
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a reinsurance. Are you sure that these tests should be deleted? Isn't it enough just to remove testScheduler?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, they're invalid tests now. This would test that it would start with 's' and then an instance of testScheduler. :)

Comment on lines 88 to 90
for (let i = 0; i < length && !subscriber.closed; i++) {
subscriber.next(array[i]);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (let i = 0; i < length && !subscriber.closed; i++) {
subscriber.next(array[i]);
}
for (let i = 0; i < length; i++) {
if (subscriber.closed) {
return;
}
subscriber.next(array[i]);
}

I double-checked and made sure that if you do not return inside the loop, then onStoppedNotification will be called.

config.onStoppedNotification = (notification) => {
  console.log('onStoppedNotification', notification);
};

EMPTY.pipe(startWith(1, 2, 3), take(2)).subscribe();

- Removes deprecated signature with scheduler
+ Slightly improves performance by allocating fewer objects during subscription

BREAKING CHANGE: `startWith` no longer accepts a scheduler. If you would like to schedule emissions in this manner, instead of `source$.pipe(startWith(1, 2, 3, scheduler)) use `concat(scheduled([1, 2, 3], scheduler), source$)`.
@benlesh benlesh force-pushed the startWith-remove-scheduler-arg branch from 95f8d95 to 13e19fc Compare May 25, 2023 17:54
@benlesh benlesh merged commit bbdce3b into ReactiveX:master May 25, 2023
3 checks passed
@benlesh benlesh deleted the startWith-remove-scheduler-arg branch May 25, 2023 18:38
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

Successfully merging this pull request may close these issues.

None yet

3 participants