Incorrect types for preview2-shim package

[konall]

Hi there, thanks for all the work on this great project!

Unless I'm missing something, it seems that the types for the preview2-shim package don't match the implementation.

Would it be possible to release a new version of the package with matching types please? Let me know if it would be helpful for me to submit a PR.

Thanks!

[vados-cosmonic]

Hey @konall thanks for pointing out the issue! A PR would definitely be appreciated -- I can make sure to review it ASAP and get it merged.

[konall]

In putting this PR together I've found that the instantiate function generated from jco transpile expects an argument of type ImportObject that doesn't match the WASIImportObject type because the object keys for ImportObject contain the version of the corresponding WASI world (eg: wasi:cli/[email protected] instead of wasi:cli/exit), causing this example from the jco book to raise a type error.

Is there a way I could address this?

[vados-cosmonic]

Ah, so that's a bit of a conundrum -- I think the best way forward for this might be to incorporate the version into the constructor (ideally, as part of the object already going in, somewhere), and use a reasonable default (usually dictated by componentize-js underneath). When building the import object we can use the version as appropriate with the interface (e.g. wasi:cli/exit), if one is supplied.

That said, I'm really surprised this was an issue -- would you mind adding a regression test along with your PR? I think the only examples we have right now must not use a versioned WASI which is odd.

[konall]

and use a reasonable default (usually dictated by componentize-js underneath).

I may be misunderstanding your suggestion, but how would preview2-shim be able to glean this information, as it's a self-contained package with no dependency on the other jco components?

[vados-cosmonic]

Unfortunately, preview2-shim can't really glean that information, what I thought you were trying to apply the fix to was the WasiShim class which generates the WasiImportObject type.

My hope was that we could use some of Typescript's advanced features (in particular template literal types) to generate the right type, on the shim side.

Does that make sense? I'm hoping we can essentially do something like this:

type WASIImportObject<Version> = {
    "wasi:cli/environment@${Version}": typeof import("./interfaces/wasi-cli-environment.d.ts");
    //...
}

[konall]

Excuse my ignorance here, just trying to wrap my head around this!

I was initially confused as to how we could arrive at a sensible default version, given that the preview2-shim package can't have any knowledge of WASI versioning- am I missing something to do with this?

I'm also wondering if this issue signals a need for broader changes, as there are other instances besides this where "version-less" imports of WASI worlds are assumed; the imports argument to the instantiate function that is generated under async instantiation, for example- especially since this is on the implementation side, not the type declaration side, I'd say this could pose a significant problem if not incorporated into these changes?

[vados-cosmonic]

No problem!

I was initially confused as to how we could arrive at a sensible default version, given that the preview2-shim package can't have any knowledge of WASI versioning- am I missing something to do with this?

So we can't really -- what we can do is mostly rev versions, and benefit from the fact that newer non-semver-breaking versions of the implementations are backwards compatible (i.e. it's fine to use @0.2.4 w/ something that expects @0.2.3). preview2-shim is free to move forward with newer and newer implementations until 0.3.0 (and we'll have preview3-shim as a package for that, actually).

I'm also wondering if this issue signals a need for broader changes, as there are other instances besides this where "version-less" imports of WASI worlds are assumed; the imports argument to the instantiate function that is generated under async instantiation, for example- especially since this is on the implementation side, not the type declaration side, I'd say this could pose a significant problem if not incorporated into these changes?

Concretely what broader changes are you envisioning here? Even a basic sketch would be nice -- would love to hear what you think here!

[konall]

Ah I see, that makes sense regarding the semver, thanks for the clarification!

By "broader changes" I meant that changing the type declarations here will apparently impact other parts of the codebase too- I'm not too familiar with the internals so it's hard for me to pinpoint exactly where to look for functionality that would break if versioned WASI imports are enforced.
It also raises other questions that I'm not sure of the official stance on, such as if omitting a version should always fail or if multiple versions of a WASI world are acceptable?

[vados-cosmonic]

By "broader changes" I meant that changing the type declarations here will apparently impact other parts of the codebase too- I'm not too familiar with the internals so it's hard for me to pinpoint exactly where to look for functionality that would break if versioned WASI imports are enforced.

Ah that's a good question -- so right now I think as long as you change only the typing, we shouldn't have a problem. While the bindings don't have the right type right now, they're still usable (we have tests for this IIRC) from JS.

It also raises other questions that I'm not sure of the official stance on, such as if omitting a version should always fail or if multiple versions of a WASI world are acceptable?

I think we can start strict and require a version to be specified and loosen in the future, if needed -- at present all WASI APIs are versioned and will be into the future, so realistically going forward no one should be trying to use an unversioned WASI API.

Multiple versions of a WASI world are certainly possible -- but I'd expect someone to make multiple WasiShim<v> (if you go that route!) for that -- and then merge them somehow. We can certainly cross that bridge when we get there.

[konall]

Having done some digging, I think I can reframe some of the points I raised; rebasing the issue for clarity:

• the type declarations for manual instantiation of preview2-shim don't match the implementation, so they need to be updated
• there is actually also a mismatch present in generated code; I can't link to it directly of course, but the ImportObject type that gets generated for the imports argument to the instantiate function expects versioned keys, whereas the generated function body accesses the imports parameter using unversioned keys - so if you adhere to the type errors your code doesn't run
• therefore, to unify the type declarations and the generated implementation as part of this change, we can either drop the versions from the type declarations or add versions to the accessing of the imports object in the generated implementation - since you mentioned wanting stricter versioning, I'm assuming the generated implementation should be changed; is this a big task?

Two other side-notes:

• the object passed to the constructor in the second example is invalid, as the ImportObject type expects fully qualified and versioned keys
• the first argument of the generated instantiate function should have | null appended to its type so that the null argument passed in the examples doesn't raise a type error

[vados-cosmonic]

Thanks for restating everything and producing a clear writeup!

therefore, to unify the type declarations and the generated implementation as part of this change, we can either drop the versions from the type declarations or add versions to the accessing of the imports object in the generated implementation - since you mentioned wanting stricter versioning, I'm assuming the generated implementation should be changed; is this a big task?

The tension here is that we want the types generated for ImportObject to have specific versions, because they do matter as a lower limit, but we want the implementation that is fed to the actual instantiate function (the imports object) to be more flexible -- taking any implementation with the appropriate shape.

The main reason we want this is so that we can use newer semver-compatible versions of preview2-shim (or any WIT interface) where an older version was expected.

Consider the case where we released a new version of preview2-shim for WASI for WASI v0.2.10 -- we want wasi:cli/[email protected] to be usable where wasi:cli/[email protected] was -- and that's made easier by chopping the version.

We do something similar with component exports themselves:

// ...
export { incomingHandler023 as incomingHandler, incomingHandler023 as 'wasi:http/[email protected]',  }

We essentially export a versionless and versionful representation of the same export.

There are at least a few ways forward here:

1. Adding versioning to accessed imports as they are used

• I'm wary of this solution because of the future-version use problem (with a naive impl we'd need an exact match), I believe this is why it was written like this in the first place -- to make versions not necessary for instantiators to worry about, with the onus on the instantiator to use a compatible implementation.
  • This can be worked around by specifying a version when generating from a WasiShim -- if we enable generating an import object with a custom version embedded (and check that the version is appropriately semver compatible)

2. Make both the versioned and unversioned keys available in the shim's generated import object.

• This satisfies the type error and functions properly but isn't particularly satisfying

IMO removing the versions from the generated instantiate is a nonstarter because that information at least denotes the lower bound of what the component expects.

I think I'm leaning towards (1) here.

I'm assuming the generated implementation should be changed; is this a big task?

I'm not sure I'd characterize it as a big task, but the potential to break things is large -- we might want to introduce this behavior and control it with a flag first. That said, the actual implementation in the codebase should be fairly straight forward (i.e. removing where we strip the versions). A lot of tests would break immediately though.