-
-
Notifications
You must be signed in to change notification settings - Fork 653
Preserve ESM for async imports to work correctly #4187
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
Conversation
Yup. It looks like you're hitting jestjs/jest#9860. As that issue says (eventually), you can solve the problem by adding However then you discover the next problem, which is that our tests rely on Anyway, I think your proposed approach of sticking to CJS for Jest is the right one, for now at least. |
richvdh
left a comment
There was a problem hiding this 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
Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com>
|
Hi @richvdh !
|
richvdh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor wording suggestions
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: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com>
Oh.. I see. I updated the description. Hope that it's correct this time. |
richvdh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks!
This example seems to have been broken by the switch to Typescript. We can't just symlink in `../..` because that gives us the typescript version of the source, which, obviously, doesn't work in node. Instead, make sure we use a prebuilt version of the js-sdk. It's actually even more broken as of js-sdk 33.0.0, thanks to the switch to ES modules (#4187), but we'll get to that later.
This example seems to have been broken by the switch to Typescript. We can't just symlink in `../..` because that gives us the typescript version of the source, which, obviously, doesn't work in node. Instead, make sure we use a prebuilt version of the js-sdk. It's actually even more broken as of js-sdk 33.0.0, thanks to the switch to ES modules (#4187), but we'll get to that later.
* Preserve ESM for async imports to work correctly (matrix-org#4187) * fix: fix lazy rust crypto import * test: use "commonjs" for tests because of circular deps * chore: revert commonjs for "module" * refactor: remove unnecessary example * refactor: add comments Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> * refactor: improve comment Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update babel.config.js --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update organization (matrix-org#4212) * v32.4.0 * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 (matrix-org#4214) * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 * Disable affected test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> * `initRustCrypto`: allow app to pass in the store key directly (matrix-org#4210) * `initRustCrypto`: allow app to pass in the store key directly ... instead of using the pickleKey. This allows us to avoid a slow PBKDF operation. * Fix link in doc-comment * Bump matrix-sdk-crypto-wasm to 5.0.0 (matrix-org#4216) Slightly more involved than normal because it requires us to pass a backup version into OlmMachine.importBackedUpRoomKeys. On the other hand we can now re-enable the test that was disabled in matrix-org#4214 due to matrix-org/matrix-rust-sdk#3447 Fixes: element-hq/element-web#27165 * Remove more deprecated methods, fields, and exports (matrix-org#4217) * Don't run migration for Rust crypto if the legacy store is empty (matrix-org#4218) * Don't run migration for Rust crypto if the legacy store is empty Fixes element-hq/element-web#27447 * Add copyright for the TypeScript files in legacy DB dumps * Provide a type for the accountPickle we check for before migration * Remove redundant backup response This is unused * Simplify keys response * Downgrade log message. --------- Co-authored-by: Richard van der Hoff <richard@matrix.org> * v33.0.0-rc.0 * v33.0.0 --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: R Midhun Suresh <hi@midhun.dev> Co-authored-by: RiotRobot <releases@riot.im> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Co-authored-by: Richard van der Hoff <richard@matrix.org>
* Preserve ESM for async imports to work correctly (matrix-org#4187) * fix: fix lazy rust crypto import * test: use "commonjs" for tests because of circular deps * chore: revert commonjs for "module" * refactor: remove unnecessary example * refactor: add comments Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> * refactor: improve comment Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update babel.config.js --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update organization (matrix-org#4212) * v32.4.0 * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 (matrix-org#4214) * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 * Disable affected test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> * `initRustCrypto`: allow app to pass in the store key directly (matrix-org#4210) * `initRustCrypto`: allow app to pass in the store key directly ... instead of using the pickleKey. This allows us to avoid a slow PBKDF operation. * Fix link in doc-comment * Bump matrix-sdk-crypto-wasm to 5.0.0 (matrix-org#4216) Slightly more involved than normal because it requires us to pass a backup version into OlmMachine.importBackedUpRoomKeys. On the other hand we can now re-enable the test that was disabled in matrix-org#4214 due to matrix-org/matrix-rust-sdk#3447 Fixes: element-hq/element-web#27165 * Remove more deprecated methods, fields, and exports (matrix-org#4217) * Don't run migration for Rust crypto if the legacy store is empty (matrix-org#4218) * Don't run migration for Rust crypto if the legacy store is empty Fixes element-hq/element-web#27447 * Add copyright for the TypeScript files in legacy DB dumps * Provide a type for the accountPickle we check for before migration * Remove redundant backup response This is unused * Simplify keys response * Downgrade log message. --------- Co-authored-by: Richard van der Hoff <richard@matrix.org> * v33.0.0-rc.0 * v33.0.0 --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: R Midhun Suresh <hi@midhun.dev> Co-authored-by: RiotRobot <releases@riot.im> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Co-authored-by: Richard van der Hoff <richard@matrix.org>
* Bringing develop to v 33.0.0 (#5) * Preserve ESM for async imports to work correctly (matrix-org#4187) * fix: fix lazy rust crypto import * test: use "commonjs" for tests because of circular deps * chore: revert commonjs for "module" * refactor: remove unnecessary example * refactor: add comments Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> * refactor: improve comment Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update babel.config.js --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update organization (matrix-org#4212) * v32.4.0 * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 (matrix-org#4214) * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 * Disable affected test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> * `initRustCrypto`: allow app to pass in the store key directly (matrix-org#4210) * `initRustCrypto`: allow app to pass in the store key directly ... instead of using the pickleKey. This allows us to avoid a slow PBKDF operation. * Fix link in doc-comment * Bump matrix-sdk-crypto-wasm to 5.0.0 (matrix-org#4216) Slightly more involved than normal because it requires us to pass a backup version into OlmMachine.importBackedUpRoomKeys. On the other hand we can now re-enable the test that was disabled in matrix-org#4214 due to matrix-org/matrix-rust-sdk#3447 Fixes: element-hq/element-web#27165 * Remove more deprecated methods, fields, and exports (matrix-org#4217) * Don't run migration for Rust crypto if the legacy store is empty (matrix-org#4218) * Don't run migration for Rust crypto if the legacy store is empty Fixes element-hq/element-web#27447 * Add copyright for the TypeScript files in legacy DB dumps * Provide a type for the accountPickle we check for before migration * Remove redundant backup response This is unused * Simplify keys response * Downgrade log message. --------- Co-authored-by: Richard van der Hoff <richard@matrix.org> * v33.0.0-rc.0 * v33.0.0 --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: R Midhun Suresh <hi@midhun.dev> Co-authored-by: RiotRobot <releases@riot.im> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Co-authored-by: Richard van der Hoff <richard@matrix.org> * Bring develop to v33.0.0 (#6) * Preserve ESM for async imports to work correctly (matrix-org#4187) * fix: fix lazy rust crypto import * test: use "commonjs" for tests because of circular deps * chore: revert commonjs for "module" * refactor: remove unnecessary example * refactor: add comments Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> * refactor: improve comment Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update babel.config.js --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> * Update organization (matrix-org#4212) * v32.4.0 * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 (matrix-org#4214) * Update dependency @matrix-org/matrix-sdk-crypto-wasm to v4.10.0 * Disable affected test Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> --------- Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> * `initRustCrypto`: allow app to pass in the store key directly (matrix-org#4210) * `initRustCrypto`: allow app to pass in the store key directly ... instead of using the pickleKey. This allows us to avoid a slow PBKDF operation. * Fix link in doc-comment * Bump matrix-sdk-crypto-wasm to 5.0.0 (matrix-org#4216) Slightly more involved than normal because it requires us to pass a backup version into OlmMachine.importBackedUpRoomKeys. On the other hand we can now re-enable the test that was disabled in matrix-org#4214 due to matrix-org/matrix-rust-sdk#3447 Fixes: element-hq/element-web#27165 * Remove more deprecated methods, fields, and exports (matrix-org#4217) * Don't run migration for Rust crypto if the legacy store is empty (matrix-org#4218) * Don't run migration for Rust crypto if the legacy store is empty Fixes element-hq/element-web#27447 * Add copyright for the TypeScript files in legacy DB dumps * Provide a type for the accountPickle we check for before migration * Remove redundant backup response This is unused * Simplify keys response * Downgrade log message. --------- Co-authored-by: Richard van der Hoff <richard@matrix.org> * v33.0.0-rc.0 * v33.0.0 --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: R Midhun Suresh <hi@midhun.dev> Co-authored-by: RiotRobot <releases@riot.im> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Co-authored-by: Richard van der Hoff <richard@matrix.org> --------- Signed-off-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Signed-off-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Bayyr Oorjak <the.bayyr.oorjak@gmail.com> Co-authored-by: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Co-authored-by: R Midhun Suresh <hi@midhun.dev> Co-authored-by: RiotRobot <releases@riot.im> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Michael Telatynski <7t3chguy@gmail.com> Co-authored-by: Andy Balaam <andy.balaam@matrix.org> Co-authored-by: Richard van der Hoff <richard@matrix.org>
Checklist
public/exportedsymbols have accurate TSDoc documentation.Fixes #4154
There were a couple of things I encountered during this PR:
import typeinsrc/models/MSC3089TreeSpace.tsbecause@babel/preset-typescriptrefused to remove that unused import. It contains two interfaces that should be removed afteryarn build. But I guess this https://github.com/matrix-org/matrix-js-sdk/blob/develop/src/models/MSC3089TreeSpace.ts#L83prevents it from being removed.
import typefixes this with minimum amount of effort.jestrefused to run spec files with ESM enabled withbabel-jest. It kept throwing this erroreven tho I have
babelandbabel-typescriptconfigured properly. I tried as described in the docs https://jestjs.io/docs/ecmascript-modules , but no success.Then I tried
ts-jestinstead ofbabel-jest. It worked but 7 of 137 specs failed. This is one of themAccording to this kulshekhar/ts-jest#1873 it might be related to circular dependencies problem since
src/crypto/index.tsends up importingsrc/client.tswhich tries to import./cryptoso the test fails. It works fine withyarn buildtho. I'm not familiar withjestso any help here will be appreciated.Anyway, I replaced
.babelrcwithbabel.config.jsto run tests withmodules: "commonjs"to makejestwork.And finally I added an example to test async imports (
rust-cryptoimport) and see that they were splitted from the main chunk. A command to build and serve the exampleSigned-off-by: Bayyr Oorjak the.bayyr.oorjak@gmail.com