matrix-org / matrix-js-sdk

Matrix Client-Server SDK for JavaScript
Apache License 2.0
1.49k stars 578 forks source link

Preserve ESM for async imports to work correctly #4187

Closed ms-dosx86 closed 1 month ago

ms-dosx86 commented 2 months ago

Checklist

Fixes https://github.com/matrix-org/matrix-js-sdk/issues/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 https://github.com/kulshekhar/ts-jest/issues/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

richvdh commented 1 month ago

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 https://github.com/jestjs/jest/issues/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. (https://github.com/jestjs/jest/issues/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.

ms-dosx86 commented 1 month ago

Hi @richvdh !

richvdh commented 1 month ago

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?

ms-dosx86 commented 1 month ago

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.