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: App context event handling for channel and membership #362

Merged
merged 8 commits into from
Feb 26, 2024

Conversation

mohitpubnub
Copy link
Contributor

@mohitpubnub mohitpubnub commented Feb 21, 2024

fix: app context events handling.

Fixes issue of App context event handling for channel and membership.

src/event-engine/index.ts Outdated Show resolved Hide resolved
src/event-engine/index.ts Outdated Show resolved Hide resolved
@mohitpubnub mohitpubnub changed the title fix: listener test falkiness fix: App context event handling for channel and membership Feb 26, 2024
@mohitpubnub mohitpubnub self-assigned this Feb 26, 2024
@mohitpubnub mohitpubnub added status: done This issue is considered resolved. priority: high This PR should be reviewed ASAP. type: fix This PR contains fixes to existing features. type: test This PR contains new tests for existing functionality or fixes to existing tests. type: refactor This PR contains refactored existing features. labels Feb 26, 2024
publishMetaData: {
timetoken: envelope.p.t,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason to rename field? It is alsready inside of publishMetaData and it doesn't look like that we need to clarify once again that it is publishTimetoken. Is there other reason for it?

Copy link
Contributor Author

@mohitpubnub mohitpubnub Feb 26, 2024

Choose a reason for hiding this comment

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

It was typo before.
In eventEmitter (the same code was there in subscriptionManager before) while creating event information object, this was the way we used to assign.

so instead of changing property name in all places (which may be error prone like we have now with channel/membership) I rename it here in subscription utility method.. same as older endpoint implementation here

in short, It's renaming to make is compatible with old implementation logic in event handling

Copy link
Contributor Author

@mohitpubnub mohitpubnub Feb 26, 2024

Choose a reason for hiding this comment

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

this can be refactored other way around than as it's there!
For consistent naming, Renamed property name in old subscribe endpoint definition as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parfeon refactored!

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.1

@techwritermat techwritermat merged commit a551a84 into master Feb 26, 2024
7 checks passed
@techwritermat techwritermat deleted the fix/test/listener branch February 26, 2024 07:51
@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
priority: high This PR should be reviewed ASAP. status: done This issue is considered resolved. type: fix This PR contains fixes to existing features. type: refactor This PR contains refactored existing features. type: test This PR contains new tests for existing functionality or fixes to existing tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants