Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

fix: Add option to subscribe immediately to an observable. #109

Merged
merged 2 commits into from Jul 18, 2021
Merged

Conversation

Jason3S
Copy link
Owner

@Jason3S Jason3S commented Jul 18, 2021

fix #106

Add lazy option to Options.
@Jason3S Jason3S merged commit 81fcec5 into main Jul 18, 2021
@Jason3S Jason3S deleted the dev branch July 18, 2021 17:36
@@ -101,4 +89,40 @@ class ReadableObservableStream<T> extends stream.Readable {
}
}
}

private subscribe() {
if (this._subscription) return;
Copy link
Owner Author

Choose a reason for hiding this comment

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

@vibl

The check is in the subscribe.

Copy link

Choose a reason for hiding this comment

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

Oops, sorry :-)

Copy link
Owner Author

@Jason3S Jason3S Jul 18, 2021

Choose a reason for hiding this comment

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

I put it here to avoid double subscription.

The if could also be in the _read() function for a tiny-tiny bit more speed, but I thought it was better to have the subscription logic in one place.

Copy link

Choose a reason for hiding this comment

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

Ok, although that makes the name of the method somewhat misleading (which is why I didn't think of looking into it).

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.

Why is subscribe() not in constructor() ? Moving it there solved a bug I had
2 participants