Typescript README/CONTRIBUTING and export more types #48
Conversation
commit: |
typescript/index.ts
Outdated
|
|
||
| export { version, Parser }; | ||
| export type { ScaledRecipeWithReport } from "./pkg/cooklang_wasm"; | ||
| export * from "./pkg/cooklang_wasm"; |
There was a problem hiding this comment.
I am hesitant to do this. We have less control over the build artifacts of wasm-pack. If we export everything, technically anything changing from this file could possibly be a breaking change. By explicitly exporting certain functions and variables, we have tighter control over the API and types. This also makes it easier to document.
| "devDependencies": { | ||
| "vite-plugin-wasm": "^3.4.1", | ||
| "vitest": "^3.2.3", | ||
| "wasm-pack": "^0.13.1" |
There was a problem hiding this comment.
The downside of switching to wasm-pack installed as a subcommand means that we don't have control over the version that is used. Considering this handles the build, keeping everyone on the same version "automatically" reduces questions around builds running differently in CI vs local vs the last time this function was run.
Was there a specific need to do this? I am not opposed to handling this through Rust, but we should identify the reasoning which prompted the change. It will improve our ability to solve that specific requirement.
Also, I would expect that the tests fail with this change, but they aren't. Presuming that is because we haven't also updated the lockfile. Seems that npm ci doesn't through on devDeps... We should probably ensure that it fails so we can confirm that our local lockfile is current.
There was a problem hiding this comment.
I did this because when I had wasm-pack included through npm, I had the following error during npm install:
npm error code 1
npm error path /Users/matt/Projects/cooklang-rs/typescript
npm error command failed
npm error command sh -c npm run build-wasm && npm run build
npm error > @cooklang/[email protected] build-wasm
npm error > wasm-pack build --target bundler
npm error Error: spawnSync /Users/matt/Projects/cooklang-rs/node_modules/wasm-pack/binary/wasm-pack Unknown system error -86
npm error at Object.spawnSync (node:internal/child_process:1120:20)
npm error at spawnSync (node:child_process:869:24)
npm error at /Users/matt/Projects/cooklang-rs/node_modules/binary-install/index.js:110:24 {
npm error errno: -86,
npm error code: 'Unknown system error -86',
npm error syscall: 'spawnSync /Users/matt/Projects/cooklang-rs/node_modules/wasm-pack/binary/wasm-pack',
npm error path: '/Users/matt/Projects/cooklang-rs/node_modules/wasm-pack/binary/wasm-pack',
npm error spawnargs: [ 'build', '--target', 'bundler' ]
npm error }
npm error npm error Lifecycle script `build-wasm` failed with error:
npm error npm error code 1
npm error npm error path /Users/matt/Projects/cooklang-rs/typescript
npm error npm error workspace @cooklang/[email protected]
npm error npm error location /Users/matt/Projects/cooklang-rs/typescript
npm error npm error command failed
npm error npm error command sh -c wasm-pack build --target bundler
I'm on macOS 15.5, node v22.17.1, npm 10.9.2. Installing wasm-pack via cargo worked for me.
don't have control over the version that is used
What do you think about using something like cargo install wasm-pack --version 0.13.1?
I would expect that the tests fail with this change, but they aren't
I think I see what you're saying. I'll try one commit with an updated lockfile without the wasm-pack dev dep and see if it fails. I wonder if we would need to add a cargo install command to the GH Action
There was a problem hiding this comment.
Hmm, have you installed rosetta? Is this a new computer? 86 is an unknown arch error from what I understand.
Using this is slightly better...
cargo install wasm-pack --version 0.13.1But it still means that every user needs to explicitly upgrade when we tell them to upgrade. Also isn't great if they use it in other projects. I wonder if we can manage it through the Cargo.toml? I don't think we could in the past, but haven't looked recently.
There was a problem hiding this comment.
I think we create an issue to follow up on this specifically. It should work with this setup, but also I am personally fine with shifting it elsewhere if we can maintain a consistent version still. Seems like we need more digging though and I don't want to block the PR on this.
panzer
changed the title
panzer
changed the title
| /* Types defined by Cooklang syntax */ | ||
| export { Section, Step, Ingredient, Cookware, Timer } from "./pkg/cooklang_wasm"; | ||
|
|
||
| /* Parsed base-types */ | ||
| export { Item, Quantity, Value } from "./pkg/cooklang_wasm"; |
There was a problem hiding this comment.
One should be able to access these from the top level ScaledRecipeWithReport. Is it purely out of convenience?
We haven't published yet so there will likely be little breakages until everything is settled. I am more fine with this than the * export at least.
Also removes unnecessary node dev-dependency wasm-pack (I had better luck installing via Rust as described in new README)