-
Notifications
You must be signed in to change notification settings - Fork 10
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
Fix splitbrain between .js
being ESM & CJS inconsistently
#189
base: main
Are you sure you want to change the base?
Conversation
would it be better to stop using |
That would also be possible but I somehow hate the idea of having to rename the output files of wasm-pack Edit: ended up being explicit on the checked in files, still added |
42eae8c
to
5d2f778
Compare
@richvdh I assigned this to you since you seem to understand it more than me. Feel free to assign another one to me :-) |
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.
no actual changes to the js/cjs/mjs files
This doesn't seem quite right? Could you give the description of the PR an update, please, to give an overview of the approach? (Looks like we're now standardising on .js
meaning ESM, and .cjs
meaning CommonJS?)
Also, please can you add an entry to the changelog?
@@ -59,7 +59,7 @@ let initialised = false; | |||
* | |||
* It will throw if there is an attempt to load the module asynchronously running | |||
* | |||
* @returns {typeof import("./pkg/matrix_sdk_crypto_wasm_bg.wasm.d")} | |||
* @returns {typeof import("./pkg/matrix_sdk_crypto_wasm_bg.wasm.d", { with: { "resolution-mode": "require" } })} |
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.
Can you help me understand what exactly this is doing? I get that it's to do with https://www.typescriptlang.org/docs/handbook/release-notes/typescript-5-3.html?#stable-support-resolution-mode-in-import-types, but what's the actual outcome/why do we need this?
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.
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.
Hrm, fair enough. I assume the commandline yarn lint:types
/tsc --noEmit
showed the same error? Or was this specific to your IDE?
I'd be interested to understand why this is a problem now when it wasn't before.
tsc is now detecting node.cjs
as CommonJs where it wasn't before? (which seems odd, why would it have done that?)
Or tsc now considers ./pkg/matrix_sdk_crypto_wasm_bg.wasm.d.ts
as ESMish where it wasn't before? (seems odd, it's typescript)
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.
Hrm, fair enough. I assume the commandline yarn lint:types/tsc --noEmit showed the same error? Or was this specific to your IDE?
Same error
/opt/homebrew/bin/yarn run lint:types
yarn run v1.22.22
$ tsc --noEmit
node.cjs:83:36 - error TS1542: Type import of an ECMAScript module from a CommonJS module must have a 'resolution-mode' attribute.
83 * @returns {Promise<typeof import("./pkg/matrix_sdk_crypto_wasm_bg.wasm.d")>}
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Found 1 error in node.cjs:83
error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
Process finished with exit code 2
Or tsc now considers ./pkg/matrix_sdk_crypto_wasm_bg.wasm.d.ts as ESMish where it wasn't before? (seems odd, it's typescript)
Yes exactly that, because .mts
and .cts
exist, .ts
esm/cjs-ness also depends on package.json type
field.
Have updated the description
Could you please point me to any guidance on doing so? Not seeing anything in the contributing doc |
https://github.com/matrix-org/matrix-rust-sdk/blob/main/CONTRIBUTING.md#writing-changelog-entries is the most relevant guide. TL;DR though: if you could just add an entry under the "UNRELEASED" heading, following the same style as previous entries, that would be be great. |
Fixes #188
no actual changes to the
js/cjs/mjs
files barring some ts annotationsSwitches the package type to ESM as that is what wasm-pack is outputting
Convinces the wasm bindgen test to use an ESM runner
Uses explicit extensions on the entrypoints between CJS/ESM