refactor: Migrate from @rocket.chat/sdk to @rocket.chat/ddp-client#1308
refactor: Migrate from @rocket.chat/sdk to @rocket.chat/ddp-client#1308Aryan-Verma-999 wants to merge 6 commits into
Conversation
| @@ -0,0 +1,17 @@ | |||
| diff --git a/lib/index.mjs b/lib/index.mjs | |||
There was a problem hiding this comment.
Please remove the patch file.
| "X-User-Id": userId, | ||
| }, | ||
| method: "POST", | ||
| await this._syncRestCredentials(); |
There was a problem hiding this comment.
Why we need to sync again ?
| }, | ||
| method: "POST", | ||
| await this._syncRestCredentials(); | ||
| return await this.sdk.rest.post("/v1/users.update", { |
There was a problem hiding this comment.
When you already defined restRequest, why use sdk.rest.post here ?
There was a problem hiding this comment.
See other similar problems, and make sure to use DRY
There was a problem hiding this comment.
got it, fixing all the _syncRestCredentials() + sdk.rest.get/post() occurrences now to go through _restRequest()
| ); | ||
| return await response.json(); | ||
| } catch (err) { | ||
| await this._syncRestCredentials(); |
There was a problem hiding this comment.
Why are we syncing everytime, can't we define it as global state and unless something related to user changes, we shouldn't sync everytime. If i am misunderstanding something , let me know
There was a problem hiding this comment.
we are doing it pre-request rather than caching because the auth token can be refreshed mid-session(token rotation). this is the same pattern rc's own sdk clients use. plus it just reads auth.getCurrentUser() (local async store lookup) and calls setCredentials()
There was a problem hiding this comment.
Couldn't we try caching approach and only run a pre-request if the cache fails? I might be mistaken, so feel free to verify if this is feasible.
There was a problem hiding this comment.
(local async store lookup) -> if this is local async store lookup, how is it always updated, who updates it mid session ?
There was a problem hiding this comment.
(local async store lookup) -> if this is local async store lookup, how is it always updated, who updates it mid session ?
this.currentUser is an in-memory object on the RocketChatAuth class instance, not localStorage. It gets updated whenever:
- loginWithPassword / loginWithResumeToken / loginWithOAuthServiceToken resolves → calls setUser() → updates this.currentUser
- The 59-minute auto-refresh fires inside getCurrentUser() itself → calls loginWithResumeToken → updates this.currentUser
There was a problem hiding this comment.
Couldn't we try caching approach and only run a pre-request if the cache fails? I might be mistaken, so feel free to verify if this is feasible.
we can do this, working to implement this
There was a problem hiding this comment.
If your sync function simply wraps the rc auth class instance that automatically refreshes when user data changes and otherwise serves from cache, then we are covered. Since the setCredentials function doesn't trigger an actual API call, there's no need to build a separate caching mechanism if the rc class is already managing it.
| } catch (err) { | ||
| console.log(err); | ||
| await this._syncRestCredentials(); | ||
| return await this.sdk.rest.get("/v1/chat.getThreadMessages", { tmid }); |
There was a problem hiding this comment.
Same comment use restRequest
| return await messages.json(); | ||
| } catch (err) { | ||
| console.log(err); | ||
| await this._syncRestCredentials(); |
There was a problem hiding this comment.
Why need to sync again
| }, | ||
| method: "POST", | ||
| await this._syncRestCredentials(); | ||
| return await this.sdk.rest.post("/v1/chat.sendMessage", { |
There was a problem hiding this comment.
Same, its repeated across many code blocks
| ); | ||
| this.setUser(response.data); | ||
| this.notifyAuthListeners(); | ||
| // Note: setUser → save() already calls notifyAuthListeners(). |
There was a problem hiding this comment.
Got it, please remove comments
| setUserAvatarUrl(res.me.avatarUrl); | ||
| setAuthenticatedUserUsername(res.me.username); | ||
| setIsUserAuthenticated(true); | ||
| // Do NOT call setIsUserAuthenticated(true) here. |
| setAuthenticatedUserId(me._id); | ||
| setAuthenticatedName(me.name); | ||
| setAuthenticatedUserRoles(me.roles); | ||
| // currentUser shape differs by login method: |
|
Hello @Aryan-Verma-999, please resolve the comments and mark the PR as ready once everything has been thoroughly tested. Also, include videos demonstrating the working system for all major cases affected by this change. Thanks. |
…rning fixes, and pinned messages features
Aryan-Verma-999
force-pushed
the
api-modernisation-ddp-client
branch
from
6745d99 to
7a7f674
Compare
2 participants
Migrate from @rocket.chat/sdk to @rocket.chat/ddp-client
Acceptance Criteria fulfillment
@rocket.chat/sdkwith the new official@rocket.chat/ddp-clientpackage.EmbeddedChatApi.tsto initializeDDPSDK.create(host)and establish standard DDP WebSocket sessions.sdk.rest.getandsdk.rest.posthelper methods._syncRestCredentials()to synchronize session tokens viasdk.rest.setCredentialson mount and re-authentications..call(...)invocations, replacing them with modern, standard REST endpoints.DOMException) on caught responses by preventing direct raw Response logs.Known Issue (Working on it): Attachment uploads and previews are successfully sent/uploaded, but their browser media previews yield 403 Forbidden because raw GET asset requests to
/file-upload/...do not carry WebSocket auth headers. I am actively working on a secure solution in a separate, dedicated PR.Fixes # (issue)
Video/Screenshots
PR Test Details
Note: The PR will be ready for live testing at https://rocketchat.github.io/EmbeddedChat/pulls/pr-1308 after approval. Contributors are requested to replace
<pr_number>with the actual PR number.