-
Notifications
You must be signed in to change notification settings - Fork 120
Conversation
|
Implementation in the RocketChat side: RocketChat/Rocket.Chat#19228 |
d-gubert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very good overall!
One thing I'm thinking is that we may leave the "setTimeout pattern" for the public API, but keep the bridge simply passing data around. This would mean having the bridge method receive the options saying whether the user is still typing, and having the Notifier accessor make the return cancellation-function in the typing method.
The reason for this is that we keep communication between the Engine and Rocket.Chat transferring simple data types (strings, objects, maybe even Buffers) instead of a complex construct like a function or an instance of a specific class, which hold more than simple data. Think about how you can't reliably serialize a function in order to transfer it over the network, if it was necessary.
@thassiov @sampaiodiego would like your opinions here too :)
|
I agree. Passing functions around can make it a bit more difficult to reason about the functionality, too (at least in my head it does). But it works, so π |
Codecov Report
@@ Coverage Diff @@
## alpha #332 +/- ##
==========================================
- Coverage 52.76% 50.69% -2.07%
==========================================
Files 83 75 -8
Lines 2968 2858 -110
Branches 433 436 +3
==========================================
- Hits 1566 1449 -117
- Misses 1402 1409 +7 Continue to review full report at Codecov.
|
Thanks for the inputs :) It does make sense. I've made changes, please take another review. |
What? β΅
Added a new type indicator API:
Why? π€
Closes #330
Closes #303
Links π
RFC: https://forums.rocket.chat/t/rfc-30-apps-engine-add-support-for-user-typing-indicator/8684 (private)
PS π