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

Getting the no-modules TypeScript *.d.ts Files Working #2396

Merged
merged 11 commits into from
Feb 8, 2021

Conversation

aaron-human
Copy link
Contributor

Partially based on #693.

The current --target no-modules TypeScript declaration files don't correctly describe what the JS interface looks like:

  • The functions exported via extern { and #[wasm_bindgen] are stored in the wasm_bindgen namespace, but the TypeScript declaration files don't put them in any namespace.
  • Everything else at the root level of the TypeScript file was exportd. As of TypeScript 3.8.3, that meant it wasn't visible. So those were switched over to declare statements.

This tries to fix the above by adjusting the output for just no-modules. It also removes the bunch of extra spaces at the end of the declaration file.

As an example, the output went from:

/* tslint:disable */
/* eslint-disable */
/**
* @param {number} id
*/
export function call_in(id: number): void;

export type InitInput = RequestInfo | URL | Response | BufferSource | WebAssembly.Module;

export interface InitOutput {
  readonly memory: WebAssembly.Memory;
  readonly call_in: (a: number) => void;
}

/**
* If `module_or_path` is {RequestInfo} or {URL}, makes a request and
* for everything else, calls `WebAssembly.instantiate` directly.
*
* @param {InitInput | Promise<InitInput>} module_or_path
*
* @returns {Promise<InitOutput>}
*/
export default function init (module_or_path?: InitInput | Promise<InitInput>): Promise<InitOutput>;
        

To:

declare namespace wasm_bindgen {
	/* tslint:disable */
	/* eslint-disable */
	/**
	* @param {number} id
	*/
	export function call_in(id: number): void;
	
}

declare type InitInput = RequestInfo | URL | Response | BufferSource | WebAssembly.Module;

declare interface InitOutput {
  readonly memory: WebAssembly.Memory;
  readonly call_in: (a: number) => void;
}

/**
* If `module_or_path` is {RequestInfo} or {URL}, makes a request and
* for everything else, calls `WebAssembly.instantiate` directly.
*
* @param {InitInput | Promise<InitInput>} module_or_path
*
* @returns {Promise<InitOutput>}
*/
declare function wasm_bindgen (module_or_path?: InitInput | Promise<InitInput>): Promise<InitOutput>;

Testing

When I ran cargo test and cargo test --target wasm32-unknown-unknown on my local setup, it reported all passes.

This aims to be the first step at getting the d.ts file working. The reason for this is that the TypeScript (3.8.3)
compiler ignores "declare" statements if it sees "export" statements. Not sure if that's a bug in TypeScript.

Either way, all I really did here was scan through all changes to `self.typescript` and see if it could add an "export".
The only big wild-card I found was the `typescript_custom_sections`, which I think exists just to let `wasm_buildgen`
annotations to add extra typescript to the document?
This is a bit crude, basically anything before the "init" block is put in this namespace.
Hopefully this works in general. I'm not 100% sure yet.
Based on the output JavaScript this seems to be how it works.
Then traced through the other instances where I'd switched `export` to `declare` and switched them over if they were going to end up in the same `wasm_bindgen` namespace.
@alexcrichton
Copy link
Contributor

Thanks for this! Would it be possible to add tests for this specifically too? I believe crates/typescript-tests doesn't currently use --no-modules, but perhaps a separate entry point and/or a separate test for --no-modules could be added?

Other than that the changes look great here!

@aaron-human
Copy link
Contributor Author

Ok, I'll see if I can setup some tests.

I'm hoping that `run.sh` is what the CI runs.
@aaron-human
Copy link
Contributor Author

Ok, the tests should now try to build with no-modules and then verify that TypeScript doesn't reject the built typescript_tests.d.ts. That's at least the file I usually use. Not sure if the *.bg.d.ts files also need to be tested. Or if the files need to be built against something (i.e. to produce an actual JS output file). What's in now was enough to catch when I manually dumped a junk string into typescript_tests.d.ts, so it's at least validating the syntax.

@realtimetodie
Copy link
Contributor

@alexcrichton

@alexcrichton
Copy link
Contributor

Thanks! For the test though it looks like this just asserts that it parses correctly, but could some JS be written which consumes the wasm-bindgen generated JS? That way we can ensure the expected idioms all typecheck and information is all listed as we'd expect.

…TypeScript.

So now the `*.d.ts` files will actually be build against (modified) `*.ts` files. This should be a better check than only verifying TypeScript doesn't fail when given just the `*.d.ts` files (what it did before).

Note that the `*.ts` files are generated from the `*.ts` files in `src` using some fairly simple Regular Expressions.

This skips checking the `typescript_tests_bg.wasm.d.ts` file, as I have no experience using that.
@aaron-human
Copy link
Contributor Author

@alexcrichton The tests now build the (no-modules) *.d.ts files against *.ts files very similar to those in src. The exact *.ts files were modified to try and remove the import statements and otherwise switch things away from using JS modules. I was aiming to avoid copying/committing the modified *.ts files in src; didn't want to make future maintainers have to manually keep the two variants in sync.

@aaron-human
Copy link
Contributor Author

Any chance this will get merged? It should be ready to review.

@alexcrichton
Copy link
Contributor

Er apologies I missed that this was ready!

I'm not sure what the test is doing though, what's with all the sed? I would hope that we wouldn't have to doctor the output, otherwise that seems like a bug that needs fixing in wasm-bindgen.

@aaron-human
Copy link
Contributor Author

The testing changes try to re-use the existing typescript testing that have been setup for the "--target web" case. The sed and grep calls in the script are to copy TypeScript files from "src" into a new (temporary) "src_no_modules" directory while also renaming things inside the files so they'll work with --target no-modules rather than --target web. I've added more comments so it's clearer that this is going on. Though the overview is that:

  • The files have dropped any lines with "import". Since "no-modules" has no modules, imports aren't needed.
  • The "wbg" namespace (which is where the tests imported the API to) has been replaced with "wasm_bindgen" (the namespace that "no-modules" exports into).
  • I skipped all the TypeScript test files that mentioned importing the "../pkg/typescript_tests_bg.wasm" (by excluding files with "_bg.wasm") as I don't know what that file does.

I was aiming to re-use the existing tests rather than writing up new ones, as I'm not too well versed in all of the binding generation functionality. (The project that got me to set this up really only used "call Rust from JS" and "call JS from Rust" with fairly simple argument/return types.) I also figured it'd be nice to not have separate files for testing "--target web" vs "--target no-modules".

@alexcrichton
Copy link
Contributor

Ah I see, thanks for clarifying! Sounds good to me!

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

Successfully merging this pull request may close these issues.

3 participants