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: emit sync state on sync.{start,stop} #518

Merged
merged 3 commits into from
Mar 14, 2024
Merged

Conversation

gmaclennan
Copy link
Member

fixes #510

@gmaclennan gmaclennan self-assigned this Mar 14, 2024
@gmaclennan gmaclennan requested a review from EvanHahn March 14, 2024 17:35
Copy link
Member

@achou11 achou11 left a comment

Choose a reason for hiding this comment

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

Added a non-blocking suggestion but lgtm - thanks for the quick fix!

@@ -259,3 +260,40 @@ test('no sync capabilities === no namespaces sync apart from auth', async (t) =>

await Promise.all(projects.map((p) => p.close()))
})

test('Sync state emitted when starting and stopping sync', async function (t) {
Copy link
Member

Choose a reason for hiding this comment

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

potential suggestion: add some assertions to test idempotency of start() and stop() i.e. events are not emitted for subsequent calls that do not meaningfully change sync state

maybe that's diving too much into internals though...

Copy link
Contributor

@EvanHahn EvanHahn left a comment

Choose a reason for hiding this comment

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

Gave this a 30-second scan and it looks good to me. I suspect Andrew's review was more thorough but hoping my 👍🏻 helps!

@achou11 achou11 merged commit 74d89be into main Mar 14, 2024
4 checks passed
@achou11 achou11 deleted the fix/sync-state-events branch March 14, 2024 18:05
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.

Emit sync-state event after calling SyncApi.start() and SyncApi.stop()?
3 participants