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

feat: new listener interface #359

Merged
merged 36 commits into from
Feb 21, 2024
Merged

feat: new listener interface #359

merged 36 commits into from
Feb 21, 2024

Conversation

mohitpubnub
Copy link
Contributor

@mohitpubnub mohitpubnub commented Feb 2, 2024

feat(api): adding first-class citizens to access subscription. Enabling subscription specific event listeners.

Adding channel, channelGroup, channelMetadata and userMetadata entities to be first-class citizens to access APIs related to them. Currently, access is provided only for subscription API.

@mohitpubnub
Copy link
Contributor Author

mohitpubnub commented Feb 2, 2024

  • handle case of overlapping/unique entitie's subscription while removing subscription
  • making new listener compatible when eventEngine is not enabled
  • added few deprecated fields in event object to make new listener backward compatible

Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

It is good in general, but I have some questions.

src/core/components/eventEmitter.js Show resolved Hide resolved
src/entities/ChannelMetadata.ts Outdated Show resolved Hide resolved
Comment on lines 31 to 40
subscribe() {
this.pubnub.subscribe({
channels: this.channelNames,
channelGroups: this.groupNames,
...(this.options?.cursor?.timetoken && { timetoken: this.options.cursor.timetoken }),
});
}
unsubscribe() {
this.pubnub.unsubscribe({ channels: this.channelNames, channelGroups: this.groupNames });
}
Copy link
Contributor

Choose a reason for hiding this comment

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

As I understand we currently use simplified version without entities usage tracking?

Copy link
Contributor Author

@mohitpubnub mohitpubnub Feb 12, 2024

Choose a reason for hiding this comment

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

Listeners will pass entity names to subscribe/unsubscribe call as it is.

to make this logic at one central place it's handled (assorting overlapping channels/groups)in this file. So that presence apis will also called /optimised accordingly.

  • added utility methods to assort unique/overlapping channel/groups. to make decision whether to remove/add incoming channel/group names into subscription/presence event engine.

src/entities/SubscriptionSet.ts Outdated Show resolved Hide resolved
src/entities/SubscriptionSet.ts Outdated Show resolved Hide resolved
@mohitpubnub mohitpubnub self-assigned this Feb 14, 2024
@mohitpubnub mohitpubnub added the status: done This issue is considered resolved. label Feb 14, 2024
@mohitpubnub
Copy link
Contributor Author

mohitpubnub commented Feb 19, 2024

@parfeon Ready for review.

  • Added methods for eventType specific listener registration.

Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

There is one question about abstract class implementation.

Comment on lines +6 to +11
protected abstract channelNames: string[];
protected abstract groupNames: string[];
protected abstract listener: Listener;
protected abstract eventEmitter: EventEmitter;
protected abstract pubnub: PubNub;
protected abstract options?: SubscriptionOptions;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about this and there will be no need to declare them in subclasses:

Suggested change
protected abstract channelNames: string[];
protected abstract groupNames: string[];
protected abstract listener: Listener;
protected abstract eventEmitter: EventEmitter;
protected abstract pubnub: PubNub;
protected abstract options?: SubscriptionOptions;
protected channelNames: string[] = [];
protected groupNames: string[] = [];
protected listener: Listener;
protected eventEmitter: EventEmitter;
protected pubnub: PubNub;
protected options?: SubscriptionOptions;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason is,
if we declare it like:
protected listener: Listener;

it forces us to make type Listener | undefined

Copy link
Contributor

Choose a reason for hiding this comment

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

It was a suggestion to move properties to the abstract class, so there will be no need to declare them again in all subclasses. If it is ok like we have it now, then np ;)

src/entities/SubscribeCapable.ts Show resolved Hide resolved
@parfeon parfeon self-requested a review February 20, 2024 05:42
Copy link
Contributor

@parfeon parfeon left a comment

Choose a reason for hiding this comment

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

LGTM

@mohitpubnub
Copy link
Contributor Author

@pubnub-release-bot release as v7.6.0

@techwritermat techwritermat merged commit a73bc58 into master Feb 21, 2024
7 checks passed
@techwritermat techwritermat deleted the CLEN-1785-listeners branch February 21, 2024 09:49
@pubnub-release-bot
Copy link
Contributor

🚀 Release successfully completed 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: done This issue is considered resolved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants