feat(notifier): Add makeNotifierFromSubscriber #5737

gibson042 · 2022-07-08T00:52:09Z

Description

Add makeNotifierFromSubscriber for lossy consumption of PublishKit subscriber objects via subscribeLatest (each getUpdateSince() call returns a unique already-settled promise for the latest result, each getUpdateSince(lastUpdateCount) call requests the next value [which under the covers constructs a new iterator] and returns a unique promise for it).

Also deprecates makeNotifierFromAsyncIterable.

This PR should land with a rebase rather than a squash; I have intentionally included the changes from #5695 along with an immediate reversion for posterity.

Security Considerations

None known.

Documentation Considerations

Agoric/documentation#684

Testing Considerations

More tests to come before merging.

turadg · 2022-07-08T16:18:47Z

packages/notifier/src/notifier.js

+harden(makeNotifierFromSubscriber);
+
+/**
+ * Deprecated adaptor from async iterable to notifier.


https://www.typescriptlang.org/play#example/jsdoc-deprecated

Suggested change

* Deprecated adaptor from async iterable to notifier.

* @deprecated adaptor from async iterable to notifier.

Also please explain here why it's deprecated.

I have intentionally included the changes from #5695 along with an immediate reversion for posterity.

I request that those two commits be omitted (or this be landed as squash). For master it's noise. If there's something important to convey about how we got to this point then just write it in HEAD. If you want there to be some record of the other work done, it's already in the referenced PR.

I'm anticipating the attempt to come up again, and would like the prior record to outlive that dead branch.

The branch will be dead but #5695 will endure for anyone who wants to see it.

Non-blocking but I do think the PR suffices as a record and to the point of discover as to why this is the design, I don't think commit history suffices. If that's the goal is should be written into HEAD.

packages/notifier/test/test-notifier-adaptor.js

turadg · 2022-07-08T16:24:33Z

packages/notifier/test/test-notifier-adaptor.js

+
+  const sequence = [];
+  const firstP = notifier.getUpdateSince();
+  firstP.then(_ => sequence.push('resolve firstP'));


please address the floating promises on 250 and 252

Address in what way?

make the warnings go away :)

either by awaiting or putting void to indicate it need not be awaited. (the warning string explains another option is to .catch but that seems silly here)

packages/notifier/test/test-notifier-adaptor.js

turadg · 2022-07-08T16:30:18Z

packages/notifier/test/test-notifier-adaptor.js

+// /////////////////////////////// makeNotifierFromSubscriber /////////////////////////////////
+
+/** @param {{conclusionMethod: 'finish' | 'fail', conclusionValue: any}} config */
+const makeGeometricPublishKit = ({ conclusionMethod, conclusionValue }) => {


this sounds like it's making a PublicKit but it's returning something different.

Suggested change

const makeGeometricPublishKit = ({ conclusionMethod, conclusionValue }) => {

const makeGeometricPublishHelper = ({ conclusionMethod, conclusionValue }) => {

Looks like calls have the same arguments. Why parameterize?

Suggested change

const makeGeometricPublishKit = ({ conclusionMethod, conclusionValue }) => {

const makeGeometricPublishHelper = () => {

Also looks like all tests share this first line. Once this is in its own file, it would work well to put in a hook:

test.beforeEach(t => { Object.assign(t.context, makeGeometricPublishHelper()); }

Or even,

test.beforeEach(t => { Object.assign(t.context, makeGeometricPublishHelper()); t.context.notifier = await makeNotifierFromSubscriber(t.context.subscriber); }

Renamed for clarity, but kept it outside the test context.

Makes sense. I was wrong about the parameterization

turadg · 2022-07-08T16:46:34Z

packages/notifier/src/notifier.js

+ * @param {ERef<Subscriber<T>>} subscriberP
+ * @returns {Promise<Notifier<T>>}
+ */
+export const makeNotifierFromSubscriber = async subscriberP => {


Suggested change

export const makeNotifierFromSubscriber = async subscriberP => {

export const makeNotifierFromSubscriber = async subscriber => {

I assume P is for "Promise" but that's not exactly what this is.

It's an ERef, which may be a promise. I think we do use this pattern to highlight that potential.

I've also seen P for Presence, kind of the opposite.

I don't think we have any consistent guidelines on this but I would make the case that when possible we rely on the type system for whether something is a promise or not. If there's no need to bind both the promise and the resolved value then use the simpler name. non-blocking request.

Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable. See #5695 .

Eager consumption led to infinite loops; see #5695 for context.

Fixes #5413

gibson042 · 2022-07-08T19:14:55Z

@turadg f40b7ed addresses everything except the "floating promises" concern... what kind of change are you expecting for that?

turadg

LG. I'll approve when the three new eslint warnings are resolved.

gibson042 · 2022-07-08T21:09:28Z

I didn't see the promise ones locally. Anyway, resolved and pushed.

turadg

👍

turadg · 2022-07-08T23:20:48Z

packages/notifier/src/notifier.js

+ * @param {ERef<Subscriber<T>>} subscriberP
+ * @returns {Promise<Notifier<T>>}
+ */
+export const makeNotifierFromSubscriber = async subscriberP => {


This isn't an async function:

Suggested change

export const makeNotifierFromSubscriber = async subscriberP => {

export const makeNotifierFromSubscriber = subscriberP => {

I don't think it needs to return Promise either. Can't it return the Notifier as makeNotifierFromAsyncIterable does?

turadg · 2022-07-08T23:25:20Z

Working on #5704 I ran into some issues using this so I took another look and found a few things to address:

whether it needs to be async
documentation ticket for notifiers.html
some confidence that the the ??? in 5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704 aren't due to bugs here

getUpdateSince() always consults the source subscribeAfter() rather than using a possibly-stale local cache.

turadg

🎉 I rebased my branch onto this version and the contract tests pass!

😸

* fix(notifier): Make makeAsyncIterableFromNotifier lossy Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable. See #5695 . * fix(notifier): Revert "Make makeAsyncIterableFromNotifier lossy" Eager consumption led to infinite loops; see #5695 for context. * feat(notifier): Add makeNotifierFromSubscriber Fixes #5413 * test(notifier): Update per code review * chore(notifier): Resolve lint warnings * fix(notifier): Align makeNotifierFromSubscriber with makeNotifierKit getUpdateSince() always consults the source subscribeAfter() rather than using a possibly-stale local cache. * fix master merge Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Turadg Aleahmad <turadg@agoric.com>

gibson042 requested review from turadg and michaelfig July 8, 2022 00:52

gibson042 mentioned this pull request Jul 8, 2022

fix(notifier): Make makeNotifierFromAsyncIterable lossy #5695

Closed

turadg requested changes Jul 8, 2022

View reviewed changes

turadg reviewed Jul 8, 2022

View reviewed changes

turadg mentioned this pull request Jul 8, 2022

add storage for makePublishKit #5739

Closed

gibson042 force-pushed the gh-5413-makeNotifierFromSubscriber branch from 3b14845 to 137088c Compare July 8, 2022 19:06

gibson042 added 4 commits July 8, 2022 15:08

fix(notifier): Make makeAsyncIterableFromNotifier lossy

8ff8718

Cherry-picked from gh-5413-lossy-makeNotifierFromAsyncIterable. See #5695 .

fix(notifier): Revert "Make makeAsyncIterableFromNotifier lossy"

923993f

Eager consumption led to infinite loops; see #5695 for context.

feat(notifier): Add makeNotifierFromSubscriber

e92402c

Fixes #5413

test(notifier): Update per code review

f40b7ed

gibson042 force-pushed the gh-5413-makeNotifierFromSubscriber branch from 137088c to f40b7ed Compare July 8, 2022 19:08

turadg reviewed Jul 8, 2022

View reviewed changes

chore(notifier): Resolve lint warnings

840ffda

turadg approved these changes Jul 8, 2022

View reviewed changes

turadg requested changes Jul 8, 2022

View reviewed changes

fix(notifier): Align makeNotifierFromSubscriber with makeNotifierKit

7a37ca5

getUpdateSince() always consults the source subscribeAfter() rather than using a possibly-stale local cache.

gibson042 mentioned this pull request Jul 12, 2022

notifiers.html needs to be updated since makePublishKit Agoric/documentation#684

Open

gibson042 requested a review from turadg July 12, 2022 19:30

turadg mentioned this pull request Jul 13, 2022

5346 replace uses of makeNotifierKit with makeNotifierFromSubscriber #5704

Merged

turadg approved these changes Jul 13, 2022

View reviewed changes

turadg added the automerge:squash Automatically squash merge label Jul 13, 2022

mergify bot and others added 2 commits July 13, 2022 22:39

Merge branch 'master' into gh-5413-makeNotifierFromSubscriber

a84d6c8

fix master merge

ee307b3

mergify bot merged commit 077718a into master Jul 14, 2022

mergify bot deleted the gh-5413-makeNotifierFromSubscriber branch July 14, 2022 00:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(notifier): Add makeNotifierFromSubscriber #5737

feat(notifier): Add makeNotifierFromSubscriber #5737

gibson042 commented Jul 8, 2022 •

edited by turadg

Loading

turadg Jul 8, 2022

turadg Jul 8, 2022

gibson042 Jul 8, 2022

turadg Jul 12, 2022

turadg Jul 8, 2022

gibson042 Jul 8, 2022

turadg Jul 8, 2022

turadg Jul 8, 2022

gibson042 Jul 8, 2022 •

edited

Loading

turadg Jul 8, 2022

turadg Jul 8, 2022

gibson042 Jul 8, 2022

turadg Jul 8, 2022

gibson042 commented Jul 8, 2022

turadg left a comment

gibson042 commented Jul 8, 2022

turadg left a comment

turadg Jul 8, 2022

turadg commented Jul 8, 2022

turadg left a comment

	* Deprecated adaptor from async iterable to notifier.
	* @deprecated adaptor from async iterable to notifier.

	const makeGeometricPublishKit = ({ conclusionMethod, conclusionValue }) => {
	const makeGeometricPublishHelper = ({ conclusionMethod, conclusionValue }) => {

	const makeGeometricPublishKit = ({ conclusionMethod, conclusionValue }) => {
	const makeGeometricPublishHelper = () => {

	export const makeNotifierFromSubscriber = async subscriberP => {
	export const makeNotifierFromSubscriber = async subscriber => {

feat(notifier): Add makeNotifierFromSubscriber #5737

feat(notifier): Add makeNotifierFromSubscriber #5737

Conversation

gibson042 commented Jul 8, 2022 • edited by turadg Loading

Description

Security Considerations

Documentation Considerations

Testing Considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 Jul 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibson042 commented Jul 8, 2022

turadg left a comment

Choose a reason for hiding this comment

gibson042 commented Jul 8, 2022

turadg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Jul 8, 2022

turadg left a comment

Choose a reason for hiding this comment

gibson042 commented Jul 8, 2022 •

edited by turadg

Loading

gibson042 Jul 8, 2022 •

edited

Loading