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

Preserve ESM for async imports to work correctly #4187

Merged
merged 9 commits into from
May 21, 2024

Conversation

ms-dosx86
Copy link
Contributor

@ms-dosx86 ms-dosx86 commented May 2, 2024

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

Fixes #4154

There were a couple of things I encountered during this PR:

  1. I had to add import type in src/models/MSC3089TreeSpace.ts because @babel/preset-typescript refused to remove that unused import. It contains two interfaces that should be removed after yarn build. But I guess this https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/MSC3089TreeSpace.ts#L83
declare module "../@types/media" {
    interface FileContent {
        [UNSTABLE_MSC3089_LEAF.name]?: {};
    }
}

prevents it from being removed. import type fixes this with minimum amount of effort.

  1. jest refused to run spec files with ESM enabled with babel-jest. It kept throwing this error
SyntaxError: Cannot use import statement outside a module

even tho I have babel and babel-typescript configured properly. I tried as described in the docs https://jestjs.io/docs/ecmascript-modules , but no success.

Then I tried ts-jest instead of babel-jest. It worked but 7 of 137 specs failed. This is one of them

at Object.<anonymous> (src/client.ts:235:57)
at Object.<anonymous> (src/models/thread.ts:19:1)
at Object.<anonymous> (src/models/event.ts:39:1)
at Object.<anonymous> (src/crypto/EncryptionSetup.ts:18:1)
at Object.<anonymous> (src/crypto/index.ts:36:1)
at Object.<anonymous> (spec/unit/crypto.spec.ts:7:1)

According to this kulshekhar/ts-jest#1873 it might be related to circular dependencies problem since src/crypto/index.ts ends up importing src/client.ts which tries to import ./crypto so the test fails. It works fine with yarn build tho. I'm not familiar with jest so any help here will be appreciated.

Anyway, I replaced .babelrc with babel.config.js to run tests with modules: "commonjs" to make jest work.

And finally I added an example to test async imports (rust-crypto import) and see that they were splitted from the main chunk. A command to build and serve the example

npx esbuild examples/lazy-crypto-init/index.js --bundle --splitting --outdir=examples/lazy-crypto-init --format=esm --servedir=examples/lazy-crypto-init --watch --define:global=globalThis

Signed-off-by: Bayyr Oorjak the.bayyr.oorjak@gmail.com

@ms-dosx86 ms-dosx86 requested a review from a team as a code owner May 2, 2024 09:14
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label May 2, 2024
@t3chguy t3chguy added the T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements label May 2, 2024
@richvdh richvdh self-requested a review May 16, 2024 22:01
@richvdh
Copy link
Member

richvdh commented May 17, 2024

2. jest refused to run spec files with ESM enabled with babel-jest. It kept throwing this error

SyntaxError: Cannot use import statement outside a module

Yup. It looks like you're hitting jestjs/jest#9860. As that issue says (eventually), you can solve the problem by adding extensionsToTreatAsEsm: ['.ts'] to the jest config.

However then you discover the next problem, which is that our tests rely on jest.mock to mock out entire modules, which doesn't seem to work very well either. (jestjs/jest#10025 seems related, though it doesn't entirely match the behaviour I observe).

Anyway, I think your proposed approach of sticking to CJS for Jest is the right one, for now at least.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Please could you also add a sign-off to your PR, as requested in https://github.com/element-hq/element-web/blob/develop/CONTRIBUTING.md#sign-off

examples/lazy-crypto-init/index.html Outdated Show resolved Hide resolved
babel.config.js Show resolved Hide resolved
@ms-dosx86
Copy link
Contributor Author

Hi @richvdh !

  • I agree that the example is not necessary and removed it.
  • Also added a comment as you asked. Hope you don't mind mentioning your comment there since you explained the situation very clearly.
  • And added a sign off in the last commit. Sorry, that was my bad. Didn't see it in the description checklist.

@richvdh richvdh self-requested a review May 20, 2024 10:52
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

minor wording suggestions

babel.config.js Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented May 20, 2024

And added a sign off in the last commit. Sorry, that was my bad. Didn't see it in the description checklist.

Thanks. Unfortunately that doesn't help with the earlier commits. Could you edit the description of the pull request to add a Signed-off-by line there?

Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com>

Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
@ms-dosx86
Copy link
Contributor Author

And added a sign off in the last commit. Sorry, that was my bad. Didn't see it in the description checklist.

Thanks. Unfortunately that doesn't help with the earlier commits. Could you edit the description of the pull request to add a Signed-off-by line there?

Oh.. I see. I updated the description. Hope that it's correct this time.

babel.config.js Outdated Show resolved Hide resolved
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@richvdh richvdh added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024
@richvdh richvdh added this pull request to the merge queue May 21, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 21, 2024
@richvdh richvdh added this pull request to the merge queue May 21, 2024
Merged via the queue into matrix-org:develop with commit fd3a4d4 May 21, 2024
28 checks passed
@ms-dosx86 ms-dosx86 deleted the bugfix/reduce-bundle-size branch May 22, 2024 11:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, performance boosts, user-facing improvements Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rust-crypto is included by default which leads to bundle size growth
3 participants