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

docs(notifier): Document PublishKit and deprecate NotifierKit/SubscriberKit #7429

Merged
merged 1 commit into from
Apr 24, 2023

Conversation

gibson042
Copy link
Member

Description

This is the long-overdue update of the notifier package README to deprecate NotifierKit and SubscriberKit in favor of PublishKit.

Security Considerations

n/a

Scaling Considerations

n/a

Documentation Considerations

https://docs.agoric.com/guides/js-programming/notifiers.html should be updated accordingly.

Testing Considerations

n/a

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Thanks for clearing this up.

I think we can go further by emphasizing subscribe{Each,Latest} instead of the organically-evolved earlier consumer APIs.

The code is still correct. However, Alice and Bob may each have missed either or both
of the non-final values due to NotifierKit’s lossy nature.

On yet another hand (🤷), the `subscriber` of a Publication includes both a
Copy link
Member

Choose a reason for hiding this comment

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

😆

Comment on lines +292 to +293
It is also possible to use `subscribeEach` for forward-lossless consumption of a `subscriber`
or `subscription`, and `subscribeLatest` for lossy consumption of a `subscriber` or `notifier`.
Copy link
Member

Choose a reason for hiding this comment

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

IMO, this should be the headline and not just a footnote. I think we should deprecate the getSharable*Internals() and observeIterat* manipulation and just point to using these adaptors with for-await-of, which has much clearer semantics.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you have something concrete in mind? The current structure from this PR looks like

  • PublishKit and Related Types
  • Distributed Asynchronous Iteration
  • Type Differences
    • Lossiness
      • NotifierKit
      • SubscriptionKit
      • PublishKit
    • Use Cases
  • Example
  • Distributed Operation
  • Summary

subscribeEach and subscribeLatest are mentioned in Type Differences and Example and Distributed Operation.

Comment on lines +4 to +5
asynchronous value sequences, the *PublishKit*, along with similar but
deprecated types *NotifierKit* and *SubscriptionKit*.
Copy link
Member

Choose a reason for hiding this comment

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

Is NotifierKit deprecated? It's not marked so if this PR also making that change it should include the @deprecated tag.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, AFAIK (search Slack for "deprecated notifier"). Where would you expect to see @deprecated?

@@ -1,59 +1,127 @@
# NotifierKits and SubscriptionKits
# PublishKit and Related Types
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 the primary use of this doc will be for someone what wants to understand the recommended way to use the package. The deprecated aspects are for understanding code you encounter but won't write.

As such I suggest having the README clearly document non-deprecated PublishKit (and NotifierKit?) but only mention the deprecated ones. Expect someone to understand non-deprecated before learning about them. Then have a separate document for understanding the older types, including how they work and why they were deprecated.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is currently the place to document all three, and am trying to minimize disruption to just what is necessary for including publish kits (especially since endojs/endo#1035 will prompt further changes).

Copy link
Member

@michaelfig michaelfig left a comment

Choose a reason for hiding this comment

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

Given the goal to change

just what is necessary for including publish kits (especially since endojs/endo#1035 will prompt further changes

I'm happy to approve this PR to get it merged.

@gibson042 gibson042 force-pushed the gibson-2023-04-publishkit-docs branch from 1d2e698 to 979a0ec Compare April 24, 2023 16:53
@gibson042 gibson042 added the automerge:rebase Automatically rebase updates, then merge label Apr 24, 2023
@mergify mergify bot merged commit de56bbf into master Apr 24, 2023
@mergify mergify bot deleted the gibson-2023-04-publishkit-docs branch April 24, 2023 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:rebase Automatically rebase updates, then merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants