Skip to content

SharedWorker refactor #471

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

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

SharedWorker refactor #471

wants to merge 6 commits into from

Conversation

parfeon
Copy link
Contributor

@parfeon parfeon commented Aug 6, 2025

refactor(shared-worker): reorganize SharedWorker code

Decouple and re-organize SharedWorker code for better maintainability.

perf(shared-worker): improved subscription state refresh logic

Additional query parameter (removed before sending) is added for requests triggered by user and state will be updated only for these requests.

refactor(logger): avoid similar timestamp in sequential lines

Log entry timestamp will be altered on millisecond if multiple log entries have similar timestamp (logged in fraction of nanoseconds).

Decouple and re-organize `SharedWorker` code for better maintainability.

perf(shared-worker): improved subscription state refresh logic

Additional query parameter (removed before sending) is added for requests triggered by user and
state will be updated only for these requests.

refactor(logger): avoid similar timestamp in sequential lines

Log entry timestamp will be altered on millisecond if multiple log entries have similar
timestamp (logged in fraction of nanoseconds).
@parfeon parfeon self-assigned this Aug 6, 2025
@parfeon parfeon added priority: medium This PR should be reviewed after all high priority PRs. type: refactor This PR contains refactored existing features. labels Aug 6, 2025
@pubnub-ops-terraform
Copy link

pubnub-ops-terraform commented Aug 6, 2025

🎉 Snyk checks have passed. No issues have been found so far.

security/snyk check is complete. No issues have been found. (View Details)

license/snyk check is complete. No issues have been found. (View Details)

if (channelGroups) channelGroups = channelGroups.filter((channelGroup) => !channelGroup.endsWith('-pnpres'));
if (channels) channels = channels.filter((channel) => !channel.endsWith('-pnpres'));
}
if (channelGroups) channelGroups = channelGroups.filter((channelGroup) => !channelGroup.endsWith('-pnpres'));
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be applicable to ee and non ee case both cases? OR it's just for getSubscribedChannels/Groups method?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This used for REST API call itself. Function used by: SubscriptionManager, PresenceEventEngine and EventEngine (when presence interval not set).

@mohitpubnub
Copy link
Contributor

@parfeon can we have this change in this PR to have changes consolidated to one PR

@parfeon
Copy link
Contributor Author

parfeon commented Aug 7, 2025

@mohitpubnub sure

…` change

Handle the situation when `userId` is changed without proper state changes (unsubscribe, change,
and subscribe back).

fix(shared-worker): fix subscribe/heartbeat aggregation after `auth` change

fix(shared-worker): fix race of conditions with backup timer

Fix race of conditions caused by explicit heartbeat on `auth` change and frequency throttling
logic, which prevented "backup" timer restart.
client.addEventListener(PubNubClientEvent.IdentityChange, () => this.moveClient(client), {
signal: abortController.signal,
});
client.addEventListener(
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering about a case where userId and auth both changed for given subscription then will this new request will contain updated token? along with new userId.

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean which event will be triggered PubNubClientEvent.IdentityChange or PubNubClientEvent.AuthChange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pubnub.setUserId will trigger PubNubClientEvent.IdentityChange and pubnub.setToken() will trigger PubNubClientEvent.AuthChange. But you have me a hint that there is nothing for pubnub.setAuthKey(). Order will be the same as user call those functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mohitpubnub nm, it will also dispatch PubNubClientEvent.AuthChange.

parfeon and others added 4 commits August 14, 2025 15:19
… next subscribe request takes place only after setToken(). to avoid race condition on checking authtoken vs previous subscribe
Fixed issue when user additionally calls unsubscribe on page unload and it collides with built-in
logic, which invalidates PubNub client from closed tab.

fix(shared-worker): fix issue that didn't handle properly too early heartbeat

Resolved the issue because of which requests that were too early received a response and still have
been sent.

fix(shared-worker): try to fix client timeout timer throttling

Try to resolve issues with timers getting throttled in SharedWorker and actually active PubNub
clients being mistakenly evicted from the state and the state reset for it.
Call 'invalidate' in the client manager only if the client removed in response to the timeout timer
detected that it is inactive.
@@ -253,6 +255,7 @@ export class PubNubClientsManager extends EventTarget {
this.unregisterClient(
client,
this.timeouts[client.subKey] ? this.timeouts[client.subKey].unsubscribeOffline : false,
true,
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏻
this may help with edge case

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium This PR should be reviewed after all high priority PRs. type: refactor This PR contains refactored existing features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants