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

refactor(notifier)!: no initial state for makePublishKit #5742

Merged
merged 3 commits into from
Jul 12, 2022

Conversation

turadg
Copy link
Member

@turadg turadg commented Jul 9, 2022

Extracted from #5704

Description

All publishing kits start empty: #5621

So makePublishKit shouldn't take an initial value and there's no need to have a separate makeEmptyPublishKit.

Security Considerations

--

Documentation Considerations

--

Testing Considerations

Updated

@turadg turadg requested review from erights and gibson042 July 9, 2022 00:05
Copy link
Member

@erights erights left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

Comment on lines 289 to 297
const subAll = await Promise.all([
subscriber.subscribeAfter(),
subscriber.subscribeAfter(expectedInitialCount - 1n),
subscriber.subscribeAfter(0n),
]);
const [subFirst] = subAll;
t.like(subFirst, {
head: { done: false },
publishCount: expectedInitialCount,
publishCount: 1n,
});
Copy link
Member

Choose a reason for hiding this comment

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

This test is assuming too much about the implementation details of makePublishKit rather than its external interface. We should just drop the subscribeAfter argument and publishCount assertion.

Suggested change
const subAll = await Promise.all([
subscriber.subscribeAfter(),
subscriber.subscribeAfter(expectedInitialCount - 1n),
subscriber.subscribeAfter(0n),
]);
const [subFirst] = subAll;
t.like(subFirst, {
head: { done: false },
publishCount: expectedInitialCount,
publishCount: 1n,
});
const subAll = await Promise.all([
subscriber.subscribeAfter(),
subscriber.subscribeAfter(),
]);
const [subFirst] = subAll;
t.like(subFirst, {
head: { done: false },
});

Copy link
Member Author

@turadg turadg Jul 10, 2022

Choose a reason for hiding this comment

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

This test is assuming too much about the implementation details

You mean the publicCount: 1n part? (The 0n argument is legit.)

I agree the interface doesn't guarantee that the next value will be one greater. Because there is a publish() it does guarantee that it will be greater though, right?

When I read this test in master I thought it was about the counts after publish but the first test in the file checks those. This seems to really be testing publishing particular values (of the initialValues) loop. Since there is no "initial value" anymore, and I trust there are already tests for .publish(), I'll just remove this test.

@gibson042 PTAAL

@turadg turadg force-pushed the ta/empty-makePublishKit branch from 542bbe2 to 1938583 Compare July 10, 2022 15:33
@turadg turadg requested a review from gibson042 July 10, 2022 15:33
@turadg
Copy link
Member Author

turadg commented Jul 11, 2022

Looking at the CI output I saw a number of floating promise warnings. Tacking on c2271c9 to resolve those and will merge by automerge:rebase

await makePublishKit(undefined).subscriber.subscribeAfter()
).publishCount;
t.true(expectedInitialCount > 0n);
const initialValues = {
Copy link
Member

Choose a reason for hiding this comment

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

Removing this eliminates coverage that arbitrary values can be published, but that coverage probably is of little value.

Comment on lines 354 to 356
void Promise.resolve(sub1.subscribeAfter()).then(result =>
sub1FirstAll.push(result),
);
Copy link
Member

Choose a reason for hiding this comment

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

Might as well be consistent here.

Suggested change
void Promise.resolve(sub1.subscribeAfter()).then(result =>
sub1FirstAll.push(result),
);
Promise.resolve(sub1.subscribeAfter())
.then(result => sub1FirstAll.push(result))
.catch(t.fail);

Comment on lines 357 to 359
void Promise.resolve(sub1.subscribeAfter(0n)).then(result =>
sub1FirstAll.push(result),
);
Copy link
Member

@gibson042 gibson042 Jul 11, 2022

Choose a reason for hiding this comment

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

Suggested change
void Promise.resolve(sub1.subscribeAfter(0n)).then(result =>
sub1FirstAll.push(result),
);
Promise.resolve(sub1.subscribeAfter(0n))
.then(result => sub1FirstAll.push(result))
.catch(t.fail);

@turadg turadg force-pushed the ta/empty-makePublishKit branch from c2271c9 to e05b6da Compare July 11, 2022 21:57
@turadg turadg added the automerge:squash Automatically squash merge label Jul 11, 2022
@mergify mergify bot merged commit 4888cac into master Jul 12, 2022
@mergify mergify bot deleted the ta/empty-makePublishKit branch July 12, 2022 21:10
turadg added a commit that referenced this pull request Jul 14, 2022
* refactor(notifier)!: no initial state for makePublishKit

* chore: handle floating promises

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge:squash Automatically squash merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants