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

rust-crypto is included by default which leads to bundle size growth #4154

Closed
ms-dosx86 opened this issue Apr 10, 2024 · 4 comments · Fixed by #4187
Closed

rust-crypto is included by default which leads to bundle size growth #4154

ms-dosx86 opened this issue Apr 10, 2024 · 4 comments · Fixed by #4187
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust T-Enhancement

Comments

@ms-dosx86
Copy link
Contributor

Reproduction: https://github.com/ms-dosx86/matrix-js-sdk-example

Steps to reproduce:

  1. npm i
  2. ng build

image

An empty project with just createClient() weighs 8.89mb.

You can also do ng build --stats-json and upload dist/matrix-js-sdk-example/stats.json to https://esbuild.github.io/analyze/ to see what causes such huge bundle size.

image

This happens because of lazy rust-crypto import here

https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/client.ts#L2255

it looks fine, the source of problem is babel preset module: 'commonjs'

https://github.com/matrix-org/matrix-js-sdk/blob/develop/.babelrc#L10

which makes the lazy import not quite lazy.

Possible workaround for those who is facing the same problem:

  1. Clone matrix-js-sdk
  2. Set module: false and node: 18 in .babelrc
  3. Replace this line https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/MSC3089TreeSpace.ts#L38 with import type { EncryptedFile, FileContent } from "../@types/media";
  4. yarn build
  5. Copy lib and replace with it node_modules/matrix-js-sdk/lib.
@richvdh
Copy link
Member

richvdh commented Apr 29, 2024

I'm afraid I'm not following everything you're saying here; I'm not that familiar with all the finer details of babel and the javascript ecosystem, and I've never used ng.

It does seem a problem that the transpiled js-sdk forces the rust-crypto import to be resolved at load time. (We don't encounter this in element-web because we use the typescript source in src, rather than the transpiled code in lib.) And yes, it seems like we need to change the module setting.

A couple of questions about your solution:

Set module: false and node: 18 in .babelrc

I think updating node makes sense, but I don't entirely follow why it is necessary here. Please could you explain?

3. Replace this line https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/MSC3089TreeSpace.ts#L38 with import type { EncryptedFile, FileContent } from "../@types/media";

Again, I don't understand why this is necessary. Are you saying that importing @types/media somehow adds a dependency on matrix-sdk-crypto? How so?

@ms-dosx86
Copy link
Contributor Author

Hi @richvdh ! I was trying to hotfix the problem, so don't take this workaround seriously as a solution.

  1. You're right, setting node: 18 is not necessary, only need to set modules: false to preserve ESM.
  2. The reason why I added import type there is because my attempt to build matrix-js-sdk locally failed. For some reason tsc didn't remove those interface imports. There are no interfaces at runtime hence there is nothing to import from "../@types/media" and build attempts fail. import type forces tsc to get rid of such imports at build time. This is probably just a misconfiguration error and should not be considered as necessary as well.

@richvdh
Copy link
Member

richvdh commented Apr 30, 2024

Why don't you open a PR setting modules: false and we can discuss further on it?

@ms-dosx86
Copy link
Contributor Author

@richvdh Sure. Here is a PR

@MidhunSureshR MidhunSureshR added T-Enhancement A-Element-R Issues affecting the port of Element's crypto layer to Rust labels May 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Element-R Issues affecting the port of Element's crypto layer to Rust T-Enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants