-
Notifications
You must be signed in to change notification settings - Fork 76
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
chore: add webidl example #544
chore: add webidl example #544
Conversation
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 a great start! Will be so useful to have this.
I think npm install
is not enough here due to the Rust step in getting webidl2wit
running. Would be great to see the steps fleshed out on that.
Then to have a test JS file that can show how the imports get picked up and describing the two Jco conventions used for the WIT conversion per https://bytecodealliance.github.io/jco/transpiling.html#experimental-webidl-imports.
Ah yes -- so there actually wasn't a
Yup -- will do! |
7c57d93
to
3242131
Compare
This would be great to land - can we get the example running on CI? |
Hey @guybedford so I got this working -- it requires a few things though:
I'm going to make a separate PR for the jco fix so it can be reviewed by itself, but wanted to note it here (it all works in this PR) |
5a15437
to
4f2113e
Compare
275aff4
to
a935366
Compare
Hey @guybedford this PR is now only blocked on merge + release of #584 (webidl2wit fixes were doable on this side!), since examples use the published |
a935366
to
f8bb74c
Compare
With this working, were we going to add it to CI like the others? |
5098ed3
to
9287851
Compare
Yeah so we can't actually test demo.js until a new jco version is released with #584 -- I was going to do that as a follow up post-release. What I can do though is make sure the compilation works, so I've added that. |
9287851
to
fdc6349
Compare
@vados-cosmonic we should be able to run all the examples against the latest Jco in the main workspace itself which should then link in the local preview2-shims from the workspace as well. I think all you need to do is set the |
Ah good idea -- will use a relative dep in the WebIDL example! [EDIT] - Done! |
22cdc96
to
3515f37
Compare
Alternatively try adding the examples folders to the workspace itself. |
Perhaps something like:
in https://github.com/bytecodealliance/jco/blob/main/package.json#L77, then keeping the normal Jco dependency definition. |
Yeah that's even better since we're trying to move to workspaces anyway -- thanks for the suggestion |
fd4e102
to
a23e2ab
Compare
Hey @guybedford actually I don't think this will work -- we don't currently build the local jco to run the examples (we run pretty quickly with the published version) -- What I'm doing is refactoring a bit so we do one |
75e2552
to
31347a3
Compare
0a39eb2
to
10572c1
Compare
I've posted #592 demonstrating just the workspaces part here, rebasing to that might help. |
Yeah, so I tried a bunch of stuff and was able to get it partially working as you did there -- the problems that are left should be trivially fixable, but aren't:
I just don't think npm workspaces work very well with how the repo is set up yet -- it's incredibly unintuitive/obvious things don't work on the first try. I'm not sure the juice is worth the squeeze in this particular PR. Maybe the workspace integration should be handled separately from this ticket -- making it work is introducing a ton of noise, and if someone pulls the repo to run an example I don't think they'd want to do an |
I landed the layering here for workspaces in #592 which should avoid the concerns mentioned. |
b7bd0bb
to
908fcfe
Compare
This commit adds an example that showcases `webidl2wit` usage to generate `jco` components. Signed-off-by: Victor Adossi <[email protected]>
908fcfe
to
30c4d80
Compare
Signed-off-by: Victor Adossi <[email protected]>
e2c536e
to
d448422
Compare
Signed-off-by: Victor Adossi <[email protected]>
c361ed8
to
742fcd5
Compare
Signed-off-by: Victor Adossi <[email protected]>
c194d9f
to
89b4b7b
Compare
Hey @guybedford this should be ready for review now, I've rebased to the workspace integration (thanks!) and tests are 🟢 |
No description provided.