Skip to content
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

Ts pdk #473

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Ts pdk #473

wants to merge 8 commits into from

Conversation

Phault
Copy link
Contributor

@Phault Phault commented May 7, 2024

Life just hit me hard, so I'm going to quickly get this PR up for you to provide feedback on or land for me if I end up going AWOL. To be clear I do intend to finish this. Pretty sure I ended last weekend with it being functionally complete, and I was just planning to add docs, tests and integrating it with the CI.

All the serde rename stuff in f3da954 was a temporary workaround to the get proper casing in the generated types. I hope you have a better solution for that part.

All the Extism declarations I hope to upstream, so you won't have to maintain it.

There's some leftover TODO comments in there, where I'd like some input. I'll add some more context in a sec.

binaryen will have to also be installed in the CI pipeline, as well as extism-js unless we run proto use in the package folder.

As mentioned there's a lack of tests, and also no counterpart for the pdk-test-utils package.

For the worst offenders of missing optionality I did end up manually overriding it, to make it less horrible DX wise.

Anyway, hope you like it and sorry for dropping a 4k lines pr for you to review :p


There's some extra context in comments for #467 that I won't copy over :)

@Phault
Copy link
Contributor Author

Phault commented May 7, 2024

Damn, looks like rebasing broke something. I'll have to check up on that later.

@Phault Phault marked this pull request as draft May 9, 2024 10:43
Phault added 7 commits May 10, 2024 11:39
This is to avoid schematic relying on its default casing, which does
not match serde's default of not touching casing.

Not super happy about this, as it cannot be overridden aside from adding
`serde(rename = ...)` to fields, but fastest way to unblock the TS PDK.
Found a few issues with the new package, which I've worked around for
now. Fixes to upstream is underway.
I'll need to look into why minification completely breaks the build.
Assuming it might be renaming stuff it shouldn't.
Since devDeps aren't installed for transitive dependencies, these would
be missing for plugin developers.

The test package was relying on phantom dependencies.
I added a lint task which uncovered that the test plugin lacked types
for `@types/node/path` which `pathe` relies on. This is mostly an issue
because `pathe` relies this type package, but has it marked as a devDep.

I thought I had fixed this with the reference directive, but turns out
tsc strips that from the .d.ts in the declaration file. They're still
in the built js file, which is how I assume it was somehow semi-working.

The fix uses the new `preserve` attribute for triple-slash directives
added in typescript 5.5 which is still in beta.
@Phault
Copy link
Contributor Author

Phault commented May 10, 2024

Waiting on a few PRs for https://github.com/extism/js-pdk to get merged and released, so I can remove a few hacks. Otherwise think it just needs CI integration as well as a proper fix for the casing of the generated api types, as the serde rename workaround I applied seems nasty.

More tests and a test-utils package would of course also be nice, but I think I'm spent for now.

@milesj
Copy link
Contributor

milesj commented May 16, 2024

I made some work on optional TS properties, but it was too over-zealous. I really need to find a way to configure it on a granular level, since we really only need optional fields for the Output types.

extism-js can get the same treatment, if extism/js-pdk#77 lands.
@Phault
Copy link
Contributor Author

Phault commented May 18, 2024

I made some work on optional TS properties, but it was too over-zealous. I really need to find a way to configure it on a granular level, since we really only need optional fields for the Output types.

Yeah that makes sense.

I'm still waiting on a new extism-js release, so I can remove a few hacks.

@milesj
Copy link
Contributor

milesj commented May 31, 2024

Figured out a solution for the TS types: #493

@Phault
Copy link
Contributor Author

Phault commented Jun 1, 2024

Figured out a solution for the TS types: #493

Awesome 💪 I will rebase and remove my hacks :p

@Phault
Copy link
Contributor Author

Phault commented Jun 1, 2024

Figured out a solution for the TS types: #493

Awesome 💪 I will rebase and remove my hacks :p

@milesj Actually struggling a bit with this. That pr fixes the default vs null ordeal (which is still awesome), but not the casing issue, right?

Also got a bit sidetracked trying to use the new extism-js rc10 version, but I keep hitting wasm trap: wasm unreachable instruction executed accompanied by Assertion failed: p->ref_count > 0 (quickjs/quickjs.c: gc_decref_child: 5672). So I guess I won't update that yet. 🤷

@milesj
Copy link
Contributor

milesj commented Jun 1, 2024

Ah yeah, this was just for optional/required. Still need to figure out casing, but I have an idea.

@Phault
Copy link
Contributor Author

Phault commented Jul 6, 2024

I'm gonna suggest maybe rejecting this PR, as I don't think it would be wise for you, to take on the extra maintenance load.

Either way I had fun building it and it pushed the extism PDK a bit forward, so I don't feel it was a time waste, nor should you feel bad if you do choose to close it.

@milesj
Copy link
Contributor

milesj commented Jul 7, 2024

It's still on my list but will probably land after v1, as APIs are continually changing. Want to wait till they stabilize for good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants