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

[ECO-4115] Split usePresence hook into different hooks for entering presence and subscribing to presence events #1674

Merged
merged 8 commits into from
Mar 13, 2024

Conversation

VeskeR
Copy link
Contributor

@VeskeR VeskeR commented Mar 8, 2024

Resolves #1621

See commit messages for more details

@github-actions github-actions bot temporarily deployed to staging/pull/1674/features March 8, 2024 08:31 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/typedoc March 8, 2024 08:32 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/bundle-report March 8, 2024 08:32 Inactive
@VeskeR VeskeR force-pushed the 1621/split-use-presence-hook branch from 38740e8 to ee509ee Compare March 8, 2024 08:32
@github-actions github-actions bot temporarily deployed to staging/pull/1674/features March 8, 2024 08:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/typedoc March 8, 2024 08:33 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/bundle-report March 8, 2024 08:33 Inactive
@VeskeR VeskeR marked this pull request as ready for review March 8, 2024 09:04
@VeskeR VeskeR requested review from owenpearson and ttypic March 8, 2024 09:04
@VeskeR VeskeR changed the title Split usePresence hook into different hooks for entering presence and subscribing to presence events [ECO-4115] Split usePresence hook into different hooks for entering presence and subscribing to presence events Mar 8, 2024
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Looks good to me, I think this will be more clear for users than subscribeOnly option in usePresence. Added some comments and we need to merge #1676 first

src/platform/react-hooks/src/hooks/usePresenceListener.ts Outdated Show resolved Hide resolved
src/platform/react-hooks/src/hooks/usePresenceListener.ts Outdated Show resolved Hide resolved
src/platform/react-hooks/src/hooks/usePresenceListener.ts Outdated Show resolved Hide resolved
src/platform/react-hooks/src/hooks/usePresenceListener.ts Outdated Show resolved Hide resolved
@VeskeR VeskeR force-pushed the 1621/split-use-presence-hook branch from ee509ee to a90e9ff Compare March 12, 2024 20:45
VeskeR added 2 commits March 12, 2024 22:19
…s and events as array in `usePresence`

Update ClientPresenceConnection in FakeAblySdk used in tests to support
passing an array of keys for `subscribe` and `unsubscribe` methods.

Update ChannelPresence.triggerSubs in FakeAblySdk to check for empty
array of client subscriptions and avoid error if no subscriptions exist
for given sub type.
This provides better maintainability (for example, field `extras` was
missing in redeclared PresenceMessage, but was present in
Ably.PresenceMessage) and provides jsdocs for PresenceMessage fields in
intellisense.
@VeskeR
Copy link
Contributor Author

VeskeR commented Mar 12, 2024

@ttypic when removing eslint-disable-next-line react-hooks/exhaustive-deps from hooks, I noticed that usePresence hook also doesn't declare 'messageOrPresenceObject' parameter (initial state when entering presence) as dependency in its useEffect/useCallback, meaning it won't re-enter when this parameter changes. It gets complicated by the fact that this parameter can be an object (passed by a ref) and users can call usePresence with an object literal usePresence('channel', {foo:'bar'}) in their components, which would cause an infinite loop of rerenders if we add it as a dependency as is - on each render the new object literal is a different reference to an object.

I think we can solve it by using useRef and using deep equals to check if the value actually changed - if we even need to fix this issue.
For now I refactored the usePresence hook a bit, it still has a eslint-disable-next-line react-hooks/exhaustive-deps, but at least with a comment now explaining how it influences things.

This was the behavior before and I don't remember seeing issues raised by users regarding this problem, so we can solve it later.
Created #1688 for this.

@VeskeR VeskeR requested a review from ttypic March 12, 2024 23:52
@VeskeR VeskeR force-pushed the 1621/split-use-presence-hook branch from a1aba5a to efa3ed3 Compare March 13, 2024 12:54
@github-actions github-actions bot temporarily deployed to staging/pull/1674/features March 13, 2024 12:54 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/bundle-report March 13, 2024 12:55 Inactive
@github-actions github-actions bot temporarily deployed to staging/pull/1674/typedoc March 13, 2024 12:55 Inactive
Copy link
Collaborator

@ttypic ttypic left a comment

Choose a reason for hiding this comment

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

Looks good, left small comment

src/platform/react-hooks/src/hooks/usePresence.ts Outdated Show resolved Hide resolved
Copy link
Member

@owenpearson owenpearson left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@VeskeR VeskeR merged commit 75af2a4 into integration/v2 Mar 13, 2024
11 of 12 checks passed
@VeskeR VeskeR deleted the 1621/split-use-presence-hook branch March 13, 2024 17:34
VeskeR added a commit that referenced this pull request Mar 15, 2024
This was wrongfully renamed in 730c621. It is a remnant
of a `usePresenceEnter` name for the new hook, which was temporary used
during development in #1674,
but we decided to keep the name `usePresence`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants