Repository navigation
feat(meetings): e2ee wasm module loading - #5344
Conversation
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
chrisadubois
left a comment
chrisadubois
left a comment
There was a problem hiding this comment.
Nothing blocking but please do read through the comments.
| Metrics.sendBehavioralMetric(BEHAVIORAL_METRICS.MEETINGS_REGISTRATION_SUCCESS); | ||
|
|
||
| // Preload is best-effort and must not delay or fail Meetings registration. | ||
| this.e2eeManager?.preload().catch((error) => { |
There was a problem hiding this comment.
Do we have retries we want to manage for this?
There was a problem hiding this comment.
it's using webex.request() that already does retries automatically and additionally if preload fails, we attempt the load again 2nd time when you do an e2ee meeting join
|
|
||
| // Construct after READY because the manager uses the initialized webex client and config. | ||
| // @ts-ignore | ||
| this.e2eeManager = new E2eeManager({webex: this.webex, wasmLoader: this.wasmLoader}); |
There was a problem hiding this comment.
question: structurally, do we want to have the wasm loader on the meetings/index, or should it be entirely maintained inside of the places where it's needed? I guess it's a shared cache so I get why we'd have it out, and it doesn't really matter since it's not exposed...
There was a problem hiding this comment.
yeah I was also debating this with myself, AI did it this way and I thought it's probably fine, we may want to load other wasm modules for other things in the future
| wasmLoader.preload.rejects(error); | ||
| const manager = new E2eeManager({webex: makeWebex(true), wasmLoader}); | ||
|
|
||
| await assert.isRejected(manager.preload(), /asset load failed/); |
There was a problem hiding this comment.
Can we add something to show registration completed after this? (separate test)
There was a problem hiding this comment.
this is already covered in meetings test: "does not fail registration when E2EE preloading rejects"
| * multiple distinct modules; each URL is loaded at most once and cached. Instantiated once per | ||
| * session so slow loads can be warmed ahead of when they're needed. | ||
| */ | ||
| export default class WasmLoader { |
There was a problem hiding this comment.
IMO this really does not belong inside of meetings. honestly it barely belongs in the main webex packages
There was a problem hiding this comment.
yes, agreed, I'll move it to webex-core, so it's there together with StaticAssetLoader, but I'll do it in a separate PR
| import LoggerProxy from '../common/logs/logger-proxy'; | ||
| import WasmLoader from '../common/wasm-loader'; | ||
|
|
||
| const E2EE_WASM_ASSET = 'e2ee-0.0.1.wasm'; |
There was a problem hiding this comment.
???? can we get a comment for this constant. I know we had some specific versioning and we have to update for this to work, this definitely needs a comment.
There was a problem hiding this comment.
added a comment.
github-actions
Bot
removed
the
validated
Thank you for your contribution! |
COMPLETES #SPARK-871963
This pull request addresses
Full set of remaining changes for E2EE MLS support in JS_SDK is here: #5305 but they are being split into a set of small PRs. This PR is one of them.
Here we have only the part for loading the WASM module.
by making the following changes
Change Type
The following scenarios were tested
unit tests
The GAI Coding Policy And Copyright Annotation Best Practices
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.