-
Notifications
You must be signed in to change notification settings - Fork 5
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
add support generating TS bindings #16
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: karthik2804 <[email protected]>
wasm-pkg-common = "0.5.1" | ||
wasm-pkg-client = "0.5.1" | ||
js-component-bindgen-component = { git = "https://github.com/bytecodealliance/jco" } |
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.
These two Git references should be pinned - otherwise you're at the mercy of main
randomly changing out from underneath you.
wasm-pkg-common = "0.5.1" | ||
wasm-pkg-client = "0.5.1" | ||
js-component-bindgen-component = { git = "https://github.com/bytecodealliance/jco" } |
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.
Sanity check if this is still needed? I can see where -bindgen
is used but not -bindgen-component
@@ -87,7 +91,55 @@ The expected file is {wit_path:?}"# | |||
println!("Bindings generated for Rust in {0}. You need to add the `wit-bindgen` crate to your Rust Spin app - e.g., `cargo add wit-bindgen`", self.output.to_str().expect("Failed to parse output path")); | |||
} | |||
BindingsLanguage::Ts => { | |||
todo!("generate ts") | |||
let imported_interfaces = get_imported_interfaces(&resolve, world_id); |
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.
This would seem a good opportunity to pull both this and the Rust arm out into helper functions, so that readers can follow the top-to-bottom flow of run
without needing to track where they are entering and existing lengthy case blocks.
for (name, contents) in files.iter() { | ||
let output_path = self.output.join(name); | ||
if !output_path.to_str().unwrap().contains("/interfaces/") { | ||
// // Skip non-interface files |
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.
Double comment
|
||
for (name, contents) in files.iter() { | ||
let output_path = self.output.join(name); | ||
if !output_path.to_str().unwrap().contains("/interfaces/") { |
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.
This seems like it include all files if self.output
contains /interfaces/
. Is that intentional? Or would it be better to filter on name
before constructing output_path
?
Are there any package / world / interface names that could make a naive contains
check go awry?
"Bindings generated for TypeScript in {0}.", | ||
self.output.to_str().expect("Failed to parse output path") | ||
); | ||
println!("\nMake sure to add the following interfaces to webpack as externals"); |
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.
How do I do this? Could the tool print the actual text I need to add and where to add it?
|
||
println!("\nupdate `knitwit.json` for \"{}\" to include dependency components. You would need to add the following fields:", self.component_id); | ||
println!(" - `project.worlds` array needs to contain `deps` as one of the worlds"); | ||
println!(" - `project.witPaths` array needs to contain the relative path to `<root of spin app>/.wit/components/{}`:", self.component_id); |
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.
Do we not know the root of the app?
println!(" - {0}/{1}", pkg_name, interface); | ||
} | ||
|
||
println!("\nupdate `knitwit.json` for \"{}\" to include dependency components. You would need to add the following fields:", self.component_id); |
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.
Should update
be capitalised here? Maybe this is intended to be another bullet in the "Make sure to..." list (which would be fine) but that's not how things seem to be presented at the moment...?
.filter_map(|(_k, v)| match v { | ||
wit_parser::WorldItem::Interface { id, .. } => { | ||
let i = &resolve.interfaces[*id]; | ||
let pkg_id = i.package.unwrap(); |
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.
This is infallible because...? (I assume .package
is fallible for a reason, just want to check that we know it will never fail here.)
let i = &resolve.interfaces[*id]; | ||
let pkg_id = i.package.unwrap(); | ||
let pkg = &resolve.packages[pkg_id]; | ||
Some((pkg.name.clone(), i.name.clone().unwrap_or_default())) |
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.
An empty interface name will cause surprising output in the "add this to webpack" output.
This PR uses
jco
now to generate bindings for TS and add instructions on how to setup the toolchain to consume the components in TS/JS