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.

Sign up for GitHub

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

Jump to bottom

[IMPROVE] Change user presence events to Meteor Streams #14488

Merged
merged 25 commits into from
May 21, 2019
Merged

[IMPROVE] Change user presence events to Meteor Streams #14488

merged 25 commits into from
May 21, 2019

Conversation

sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented May 13, 2019 •
edited
Loading

Closes #12800

Changing to streams will make it lighter on both client and server.

The first batch of online/away users will come from a REST request (thus will not flood websocket) and only further changes on status will come though DDP.

  • Update Presence package to include a change on clients to not call connect anymore
  • Presence package process instance removals on each instance, which generates lots of duplicate status changes if an instance goes down/updates
  • Test without UPM

@sampaiodiego sampaiodiego added this to the 1.1.0 milestone May 13, 2019
@sampaiodiego sampaiodiego changed the title Change status to streams [IMPROVEMENT] Change status to streams May 13, 2019
@sampaiodiego sampaiodiego changed the title [IMPROVEMENT] Change status to streams [IMPROVEMENT] Change user presence events to Meteor Streams May 13, 2019
@sampaiodiego sampaiodiego changed the title [IMPROVEMENT] Change user presence events to Meteor Streams [IMPROVE] Change user presence events to Meteor Streams May 14, 2019
ggazzo
ggazzo requested changes May 14, 2019
View reviewed changes
Copy link
Member

@ggazzo ggazzo left a comment

Choose a reason for hiding this comment

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

@sampaiodiego do you know if there any problem with this observe?
/client/startup/usersObserve.js because I'm not sure, but there are a lot of fields there...

just answer my questions (despite the questions, the PR looks very good :) )

imports/users-presence/server/activeUsers.js Outdated Show resolved Hide resolved
app/ui-master/client/main.js Outdated Show resolved Hide resolved
@rodrigok rodrigok added this to PRs in 1.1.0 Review May 15, 2019
@rodrigok rodrigok moved this from PRs to PR 1.1.0 in 1.1.0 Review May 15, 2019
@rodrigok rodrigok moved this from PR Backlog to In progress in 1.1.0 Review May 15, 2019
@sampaiodiego
Copy link
Member Author

@ggazzo idk, and for sure it doesn't need a reactive query for all that fields. I'm not totally sure if all that is still needed.

@ggazzo ggazzo moved this from In progress to Waiting approval in 1.1.0 Review May 16, 2019
@geekgonecrazy
Copy link
Contributor

🥰 glad to see this! I think this will help. Especially moving to a single http call to grab all statuses/presence instead of sometimes ending up being cramming down the websocket in a flood.

@sampaiodiego sampaiodiego force-pushed the change-status-to-streams-again branch from b2bb510 to 0a935c1 Compare May 16, 2019 12:28
@sampaiodiego sampaiodiego force-pushed the change-status-to-streams-again branch from 0a935c1 to c361643 Compare May 16, 2019 12:46
@sampaiodiego sampaiodiego force-pushed the change-status-to-streams-again branch from c361643 to 9c3c294 Compare May 16, 2019 13:00
geekgonecrazy
geekgonecrazy reviewed May 17, 2019
View reviewed changes
imports/startup/client/listenActiveUsers.js Outdated Show resolved Hide resolved
imports/startup/client/listenActiveUsers.js Outdated Show resolved Hide resolved
imports/startup/client/listenActiveUsers.js Outdated Show resolved Hide resolved
sampaiodiego and others added 2 commits May 16, 2019 23:55
@sampaiodiego @geekgonecrazy
Rename function getActiveUsers to getUsersPresence
9ff73a0 
Co-Authored-By: Aaron Ogle <geekgonecrazy@users.noreply.github.com>
@sampaiodiego
Improve logic to get users presence
f83d7dc
@sampaiodiego sampaiodiego force-pushed the change-status-to-streams-again branch from 867935a to f83d7dc Compare May 17, 2019 21:09
@geekgonecrazy geekgonecrazy mentioned this pull request May 20, 2019
Merged
@ggazzo ggazzo mentioned this pull request May 20, 2019
Closed
1.1.0 Review automation moved this from Waiting approval to Approved May 20, 2019
@ggazzo ggazzo merged commit 40cab8f into develop May 21, 2019
1.1.0 Review automation moved this from Approved to Done May 21, 2019
@ggazzo ggazzo deleted the change-status-to-streams-again branch May 21, 2019 00:03
@diegolmello diegolmello mentioned this pull request May 24, 2019
Merged
wreiske added a commit to wreiske/Rocket.Chat that referenced this pull request May 27, 2019
@wreiske
Merge branch 'new-reports-ui' of https://github.com/wreiske/Rocket.Chat…
a120595 
… into new-reports-ui

* 'new-reports-ui' of https://github.com/wreiske/Rocket.Chat: (97 commits)
  LingoHub Update 🚀 (RocketChat#14643)
  [FIX] Role name spacing on Permissions page (RocketChat#14625)
  [FIX] Avatar images on old Livechat client (RocketChat#14590)
  [FIX] Inject code at the end of <head> tag (RocketChat#14623)
  [FIX] Mailer breaking if user doesn't have an email address (RocketChat#14614)
  Ci improvements (RocketChat#14600)
  [FIX] E2E messages not decrypting in message threads (RocketChat#14580)
  Fix: emoji render performance for alias (RocketChat#14593)
  [FIX] Send replyTo for livechat offline messages (RocketChat#14568)
  Federation i18n message changes (RocketChat#14595)
  [REGRESSION] Fix Slack bridge channel owner on channel creation (RocketChat#14565)
  Fix thumbs up emoji shortname (RocketChat#14581)
  Fix broken logo url in app.json (RocketChat#14572)
  Add digitalocean button to readme (RocketChat#14583)
  Fix: Add emoji shortnames to emoji's list (RocketChat#14576)
  [IMPROVE] Message rendering time (RocketChat#14252)
  [IMPROVE] Change user presence events to Meteor Streams (RocketChat#14488)
  Removed unnecessary DDP unblocks (RocketChat#13641)
  [FIX] Downloading files when running in sub directory (RocketChat#14485)
  [FIX] Broken layout when sidebar is open on IE/Edge (RocketChat#14567)
  ...
@sampaiodiego sampaiodiego mentioned this pull request May 28, 2019
Merged
@AmShaegar13 AmShaegar13 mentioned this pull request Jun 27, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geekgonecrazy geekgonecrazy geekgonecrazy left review comments

@ggazzo ggazzo ggazzo approved these changes

@rodrigok rodrigok Awaiting requested review from rodrigok

Assignees
No one assigned
Labels
None yet
Projects
No open projects
1.1.0 Review
  
Done
Milestone
1.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sampaiodiego @geekgonecrazy @ggazzo