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: subscription closed does not comply with proposal #151

Merged
merged 3 commits into from
Mar 18, 2023

Conversation

naporin0624
Copy link
Contributor

Thank you for creating a great lightweight library.

I have fixed the difference between proposal-observable and the closed type in subscription.

The following are subscription types for proposal-observable.

interface Subscription {

    // Cancels the subscription
    unsubscribe() : void;

    // A boolean value indicating whether the subscription is closed
    get closed() : Boolean;
}

Current wonka subscription type.

interface ObservableSubscription {
  /** A boolean flag indicating whether the subscription is closed.
   * @remarks
   * When `true`, the subscription will not issue new values to the {@link ObservableObserver} and
   * has terminated. No new values are expected.
   *
   * @readonly
   */
  closed?: boolean;
  /** Cancels the subscription.
   * @remarks
   * This cancels the ongoing subscription and the {@link ObservableObserver}'s callbacks will
   * subsequently not be called at all. The subscription will be terminated and become inactive.
   */
  unsubscribe(): void;
}

Confirmed

  • pnpm build passes
  • pnpm check passes
  • pnpm lint passes
  • pnpn test passes

@kitten
Copy link
Member

kitten commented Mar 17, 2023

Nice catch!
I think I can see how I made this mistake. I think I wrote it as optional because of ObservableLike for fromObservable to make that type more flexible 🤔

Would you mind updating the return type here:

subscribe(observer: ObservableObserver<T>): ObservableSubscription;

To something like { unsubscribe(): void } please?
I think that'd help out a lot to keep fromObservable flexible while making toObservable more strict ✌️

@naporin0624
Copy link
Contributor Author

ok! It took me a while, but I understand what you're saying and I'll fix it right away!

Copy link
Member

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Thanks again! 🙌

@kitten kitten merged commit 95c15e7 into 0no-co:main Mar 18, 2023
@github-actions github-actions bot mentioned this pull request Mar 18, 2023
@naporin0624 naporin0624 deleted the fix_subscription_type branch March 18, 2023 05:29
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

2 participants