-
Notifications
You must be signed in to change notification settings - Fork 20
Typescript README/CONTRIBUTING and export more types #48
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
Open
panzer
wants to merge
5
commits into
cooklang:main
Choose a base branch
from
panzer:docs-and-exports
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
0516c94
Adds README for developers and rm wasm-pack from npm deps
panzer 961a2ae
Export all available types from the TS package
panzer 0376741
Add in-development note
panzer e93b7c7
Create CONTRIBUTING.md
panzer 91eda8b
Narrow exports
panzer File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| ## Prerequisites | ||
|
|
||
| This project uses Rust compiled to WebAssembly, so you'll need to set up the Rust toolchain properly before development. | ||
|
|
||
| ### Required Tools | ||
|
|
||
| 1. **Rust via rustup** (recommended over system package managers) | ||
| 2. **wasm-pack** | ||
| 3. **Node.js** | ||
|
|
||
| ### Setup Instructions | ||
|
|
||
| #### 1. Install Rust (via rustup) | ||
|
|
||
| **Important:** Use rustup rather than system package managers (Homebrew on macOS, apt on Ubuntu, etc.) for better toolchain management. | ||
|
|
||
| Install Rust using rustup. Platform specific instructions at [rustup.rs](https://rustup.rs/). | ||
|
|
||
| #### 2. Add WebAssembly Target | ||
|
|
||
| ```bash | ||
| rustup target add wasm32-unknown-unknown | ||
| ``` | ||
|
|
||
| #### 3. Install wasm-pack | ||
|
|
||
| ```bash | ||
| cargo install wasm-pack | ||
| ``` | ||
|
|
||
| #### 4. Verify Installation | ||
|
|
||
| ```bash | ||
| # Check Rust | ||
| rustc --version | ||
| rustup target list --installed | grep wasm32 | ||
|
|
||
| # Check wasm-pack | ||
| wasm-pack --version | ||
|
|
||
| # Verify WASM target is available | ||
| rustc --print target-list | grep wasm32-unknown-unknown | ||
| ``` | ||
|
|
||
| ## Development | ||
|
|
||
| ### Install Dependencies | ||
|
|
||
| From the project root: | ||
| ```bash | ||
| npm install | ||
| ``` | ||
|
|
||
| ### Playground Development | ||
|
|
||
| From the project root: | ||
| ```bash | ||
| npm run playground | ||
| ``` | ||
|
|
||
| ### Project Structure | ||
|
|
||
| - `src/` - Rust source code (extensions to cooklang-rs) | ||
| - `pkg/` - Generated WebAssembly files (created by wasm-pack) | ||
| - `index.ts` - TypeScript entry point | ||
| - `Cargo.toml` - Rust dependencies and configuration |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # @cooklang/cooklang-ts | ||
|
|
||
| A TypeScript/JavaScript library for parsing CookLang recipes, built with Rust and WebAssembly. | ||
|
|
||
| > [!NOTE] | ||
| > Package is under development. What exists here may not be published to NPM yet. | ||
|
|
||
| ## Contributing | ||
| See `CONTRIBUTING.md` |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,10 @@ | ||
| import { version, Parser } from "./pkg/cooklang_wasm"; | ||
|
|
||
| export { version, Parser }; | ||
| /* High Level Types */ | ||
| export { Parser, Recipe, version } from "./pkg/cooklang_wasm"; | ||
| export type { ScaledRecipeWithReport } from "./pkg/cooklang_wasm"; | ||
|
|
||
| /* 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"; | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
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.
The downside of switching to
wasm-packinstalled 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 cidoesn'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this because when I had
wasm-packincluded through npm, I had the following error duringnpm install:I'm on macOS 15.5, node v22.17.1, npm 10.9.2. Installing
wasm-packvia cargo worked for me.What do you think about using something like
cargo install wasm-pack --version 0.13.1?I think I see what you're saying. I'll try one commit with an updated lockfile without the
wasm-packdev dep and see if it fails. I wonder if we would need to add a cargo install command to the GH ActionThere 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.
Hmm, have you installed rosetta? Is this a new computer?
86is an unknown arch error from what I understand.Using this is slightly better...
But 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.