Skip to content
This repository was archived by the owner on Dec 3, 2025. It is now read-only.

chore: expose hasFinishedLoading#93

Merged
devceline merged 6 commits intomainfrom
chore/expose-finished-loading
Jan 10, 2024
Merged

chore: expose hasFinishedLoading#93
devceline merged 6 commits intomainfrom
chore/expose-finished-loading

Conversation

@devceline
Copy link
Copy Markdown
Contributor

@devceline devceline commented Jan 10, 2024

Changes

  • bump version to 0.16.2 (patch)
  • add new function hasFinishedLoading: Checks if watchSubscriptions has succeeded and finalized

Why

The response for watch subscriptions is sometimes received before notify client's init function has resolved.

Consumers of this SDK should be able to tell when notify client has fetched all subscriptions to show appropriate loading states.

@devceline devceline requested a review from bkrem January 10, 2024 16:49
@devceline devceline marked this pull request as ready for review January 10, 2024 16:53
@bkrem
Copy link
Copy Markdown
Contributor

bkrem commented Jan 10, 2024

The response for watch subscriptions is sometimes received before notify client's init function has resolved.

Consumers of this SDK should be able to tell when notify client has fetched all subscriptions to show appropriate loading states.

  • I understand the possible race condition here between init and watchSubscriptions
  • Why is this currently screwing over expectations for SDK consumers? E.g. if awaiting init and init happens after watchSubscriptions has already completed, why is this throwing off loading expectations?

subscriptions,
});

this.finishedInitialLoad = true;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What about the error case here? If we hit the JsonRpcError case SDK consumers will never see finishedInitialLoad === true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Annoying I know, but could we add a comment here please to highlight why this flag is needed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Making the boolean to true in error case as well for now, since it technically finished loading

@devceline
Copy link
Copy Markdown
Contributor Author

The response for watch subscriptions is sometimes received before notify client's init function has resolved.
Consumers of this SDK should be able to tell when notify client has fetched all subscriptions to show appropriate loading states.

  • I understand the possible race condition here between init and watchSubscriptions
  • Why is this currently screwing over expectations for SDK consumers? E.g. if awaiting init and init happens after watchSubscriptions has already completed, why is this throwing off loading expectations?

In web3inbox, there is an outstanding PR to show loading state while subscriptions are loading in. The way we've been telling if subscriptions are loading in is the notify_subscriptions_changed event. However, we've noticed that sometimes that event is being fired before init is complete.

The problem is that we are attaching event listeners on("notify_subscriptions_changed" after init. So by the time we attach the event listener, the event has already been fired and we missed. Therefore, we can't know when subscriptions have finished loading in.

Alternatives considered was passing event (EventEmitter) to notify client and listening to it from there but it felt unncessarily hacky

@bkrem
Copy link
Copy Markdown
Contributor

bkrem commented Jan 10, 2024

The problem is that we are attaching event listeners on("notify_subscriptions_changed" after init.

This was the piece I was missing, thank you 👍

@devceline devceline merged commit ba08d91 into main Jan 10, 2024
@devceline devceline deleted the chore/expose-finished-loading branch January 10, 2024 17:12
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants