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

Deno support #102

Closed
zifeo opened this issue Jul 1, 2023 · 17 comments
Closed

Deno support #102

zifeo opened this issue Jul 1, 2023 · 17 comments

Comments

@zifeo
Copy link

zifeo commented Jul 1, 2023

Great work! The library seems overall to work well with Deno, some syntax updates are nonetheless required:

  • add file extension like .d.ts
  • the main components.d.ts files syntax is not recognized
import { Core as CoreExports } from './exports/core';
'core': typeof CoreExports, // ← that line
@guybedford
Copy link
Collaborator

Thanks for the feedback here, can you share the example WIT and flags you were using to get the above output with 'core': typeof CoreExports? Also make sure you are running the latest release of jco as well.

@guybedford
Copy link
Collaborator

Unfortunately, it seems TypeScript itself doesn't like the .d.ts extension being explicit - see the CI error in #105:

1) Typescript compilation
       TypeScript Compilation:
     Error: test/output/flavorful/flavorful.d.ts(1,33): error TS2691: An import path cannot end with a '.d.ts' extension. Consider importing './imports/testwasi.js' instead.

@zifeo
Copy link
Author

zifeo commented Jul 4, 2023

@guybedford Thanks for looking into this. Wit can be found here and we are using jco 0.7.0 because we cannot support yet component version 3 in all hosts (waiting for Python support).

@iulian-birlica
Copy link

I saw your issue because it was linked to mine. Python support for component version 3 happened in wasmtime-py, so you can now go on with development.

@zifeo
Copy link
Author

zifeo commented Jul 17, 2023

@iulian-birlica Thanks for letting us know.

@guybedford I can confirm same issue happens with latest version.

@guybedford
Copy link
Collaborator

@zifeo if we replace the .d.ts imports with .js imports will that work here?

@zifeo
Copy link
Author

zifeo commented Jul 18, 2023

@guybedford Here is the wit and use case: Repro & WIT files with:

cargo build -p typegraph_core --target wasm32-unknown-unknown -F wasm
wasm-tools component new target/wasm32-unknown-unknown/debug/typegraph_core.wasm -o target/debug/typegraph_core.wasm
jco transpile target/debug/typegraph_core.wasm -o typegraph/deno/gen --no-nodejs-compat --map "*=../src/imports.ts"

There is a few things looking weird:

  • imports and exports seems inverted : import type { RuntimeId } from '../imports/core'; instead of from the exports folder
  • the base imports does not consider the package name (component 3) but the file name are: import type { RuntimeId } from "../imports/core"; instead of import type { RuntimeId } from "../exports/metatype-typegraph-core.d.ts";
  • imports in Deno are URL so they require the extension: I suppose a --deno flag is required
  • for the .d.ts import, there is an official cookbook : https://deno.land/manual@v1.35.1/advanced/typescript/types

What do you advise here? We currently run a bash script to fix those points.

@guybedford
Copy link
Collaborator

@zifeo I think I agree with you that an explicit --deno-compat flag would make sense here given there doesn't seem to be a supportable subset.

Are you interested in contributing to the project? Otherwise I will get to this one when I can.

@zifeo
Copy link
Author

zifeo commented Jul 19, 2023

@guybedford let's see whether we can give it a try in the next days. How should the first element be handled "imports and exports seems inverted", is that expected or is there something we are not following for that?

@guybedford
Copy link
Collaborator

@zifeo I can take a look at the other issues from the case, but couldn't get the clone to work as the deps branch does not seem to be there though?

@zifeo
Copy link
Author

zifeo commented Jul 19, 2023

@guybedford sorry, the PR got merged, can you retry now? I restored the branch :)

@guybedford
Copy link
Collaborator

@zifeo just had a look and this is the WIT I get for jco wit target/debug/typegraph_core.wasm:

package root:component

world root {
  import metatype:typegraph/abi

  export metatype:typegraph/core
  export metatype:typegraph/runtimes
}

And here is the main generated TypeScript file:

import { ImportsAbi } from './imports/abi';
import { ExportsMetatypeTypegraphCore } from './exports/metatype-typegraph-core';
import { ExportsMetatypeTypegraphRuntimes } from './exports/metatype-typegraph-runtimes';
export const core: typeof ExportsMetatypeTypegraphCore;
export const runtimes: typeof ExportsMetatypeTypegraphRuntimes;

This matches correctly - core and runtimes are exported, while abi is imported.

The file name does include the full name as well it seems.

Let me know if there's something I'm missing, otherwise I think it's just the Deno extension issue here then?

@zifeo
Copy link
Author

zifeo commented Jul 20, 2023

@guybedford look at the file exports/metatype-typegraph-runtimes.d.ts, there is a few imports with the following form '../imports/core' which points at nothing. Can you reproduce that?

@guybedford
Copy link
Collaborator

@zifeo thanks for following up on this - I finally see what you mean in that the relative type import from one typing file to another is wired up incorrectly. I've posted a dedicated issue for this specific relative type loading issue in #112 and will aim to post a fix soon.

@guybedford
Copy link
Collaborator

Thanks again for the report here. I believe the cases here were fixed, but please reopen if we're still missing anything.

@zifeo
Copy link
Author

zifeo commented Sep 29, 2023

@guybedford Sorry the late answer, the version 0.12.1 works better, thanks! There is a Deno incompatibility due to the file extension. A simple post processing is enough: .replace(/^(import .*)(?<!\.ts)\';$/, "$1.d.ts';"). Can we keep this open till we have time to contribute or until there is a deno flag?

@guybedford
Copy link
Collaborator

@zifeo with #228, we now use the .js extension in the type definitions. Do share if this helps this issue or not.

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

No branches or pull requests

3 participants