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

Question: jco types generating namespaces instead of modules? #439

Open
karthik2804 opened this issue May 21, 2024 · 12 comments · May be fixed by #528
Open

Question: jco types generating namespaces instead of modules? #439

karthik2804 opened this issue May 21, 2024 · 12 comments · May be fixed by #528
Labels
enhancement New feature or request

Comments

@karthik2804
Copy link

I am using jco types to create the typings to build an SDK based on the generated code, the generated types contain namespaces instead of modules which means IDE does not provide intellisense and also tsc fails to compile without //@ts-ignore the wit imports .

I have the following wit

package local:hello;

interface logger {
  log: func(val: string);
}

world hello {
    import logger; 
    export say-hello: func(name: string);
}

Using the following command to generate the types jco types -o types <path to wit> provides the following directory structure

types
├── hello.d.ts
└── interfaces
    └── local-hello-logger.d.ts

And the contents of the files are

// hello.d.ts
import { LocalHelloLogger } from './interfaces/local-hello-logger.js';
export function sayHello(name: string): void;

// local-hello-logger.d.ts
export namespace LocalHelloLogger {
 export function log(val: string): void;
} 

And the following is the guest code

// hello.js
import * as logger from 'local:hello/logger';

export function sayHello(name) {
  logger.log(`Hello ${name}`);
}

I could get around this by manually editing the generated types to be the following

declare module "local:hello/logger" {
  export function log(val: string): void;
}

Please let me know if I am missing something here. Thanks!

@guybedford
Copy link
Collaborator

Explicitly declaring the imports sounds like a sensible approach. The current types are primiarly for jco transpile, where the imports don't matter to the end user as they are encapsulated, so they only matter so far as they are reexported.

In this guest generation case, it definitely makes sense to generate a declare module output for all imports. It would be nice to add a new flag for this which outputs this new form. --declare-imports or similar perhaps.

@guybedford guybedford added the enhancement New feature or request label May 21, 2024
@cdata
Copy link

cdata commented May 30, 2024

I just ran into this myself. I would be happy to look into contributing the change, if you're looking for help.

@guybedford
Copy link
Collaborator

@cdata no one is currently working on this, a PR would be great if you're interested in contributing.

@karthik2804
Copy link
Author

@cdata just wanted to check in if you were working on this issue. I would be happy to look into it otherwise

@lachieh
Copy link

lachieh commented Nov 26, 2024

@guybedford Is this something that would become the default for generated types? Namespaces could work if held correctly, but modules seems like it would work better. If I'm thinking about it correctly, it would also mean that we could generate a single d.ts reference that could roll up all the modules and would mean no more manually editing tsconfig to include the path aliases?

@guybedford
Copy link
Collaborator

@lachieh thanks for starting a PR! There are two separate use cases here:

  1. For jco componentize, being able to define modules for the imports so that type checking can work for JS guest components.
  2. For jco transpile, being able to output a single types file instead of multiple.

It sounds like you're also interested in use case (1) here? But I do think defining the import types as modules should always be a flag because its not a feature normally important to the transpile use case and may in fact not match up with expected names used by transpile consumers.

Perhaps the flag should even be jco types --guest or something like that to indicate we are generating types for the guest perspective (where --host is effectively the default today).

@lachieh lachieh linked a pull request Nov 26, 2024 that will close this issue
@lachieh
Copy link

lachieh commented Nov 26, 2024

Yep, it was types for guest components.

Because the generated types come out as a namespace, the IDE expects that to be able to access the publish function, it should be imported through the namespace but jco componentize expects a module import.

world:

package acme:messaging@0.1.0;

world component {
    export wasmcloud:messaging/handler@0.2.0;

    import wasmcloud:messaging/consumer@0.2.0;
}

Gen type changed to module:

- export namespace WasmcloudMessagingConsumer {
+ export module "wasmcloud:messaging/[email protected]" {

Guest suggested import, changed to module import:

- import {WasmcloudMessagingConsumer} from 'wasmcloud:messaging/[email protected]';
+ import {publish} from 'wasmcloud:messaging/[email protected]';

@guybedford
Copy link
Collaborator

Right, then I think this makes a lot of sense as the default output for a jco types --guest or similar and this would be a huge feature to have and properly document in componentize workflows.

Feel free to ask any questions in the PR further - it would be great to get this over the line!

@lachieh
Copy link

lachieh commented Nov 26, 2024

Yeah, I'll update the PR to --guest.

The other tangentially related part I've noticed is that to actually get those types to work with TS, they have to be manually added to the tsconfig paths:

{
  // ...
  "compilerOptions": {
    // ...
    "paths": {
      "wasmcloud:messaging/[email protected]": [
        "./types/interfaces/wasmcloud-messaging-consumer.d.ts"
      ]
    }
  }
}

It'd be nice if there was a way to reference all the types without needing to manually configure the paths for each module. Any thoughts on how to make this happen? Even if it was an extra .d.ts file that includes a /// <reference /> comment, it'd be less manual per module.

@guybedford
Copy link
Collaborator

I'm not sure actually. Does it work if you define an index.d.ts that contains /// <reference /> comments? Ideally it should be a one liner include only yeah.

@lachieh
Copy link

lachieh commented Nov 26, 2024

I'll do some experimentation and report back

@lachieh
Copy link

lachieh commented Nov 27, 2024

Ok, after some poking this morning the answer is that, yes, we can get to a pretty nice experience with the module declarations by just including the generated folder in the project. The caveat is that the files must be ambient module declarations in that they must only include declare module statements and must not import/export anything at the top level.

From my example before, the generated types for wasmcloud:messaging/[email protected] after my PR look like this:

declare module "wasmcloud:messaging/[email protected]" {
  /**
   * Perform a request operation on a subject
   */
  export function request(subject: string, body: Uint8Array, timeoutMs: number): BrokerMessage;
  /**
   * Publish a message to a subject without awaiting a response
   */
  export function publish(msg: BrokerMessage): void;
}
import type { BrokerMessage } from './wasmcloud-messaging-types.js';
export { BrokerMessage };

Because of the last two lines, TS assumes the file is a module and the ambient module declarations are ignored.

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

Successfully merging a pull request may close this issue.

4 participants