ethereum / eipw

Mozilla Public License 2.0
26 stars 24 forks source link

Add TS typings #78

Open Pandapip1 opened 1 year ago

Pandapip1 commented 1 year ago

Pretty simple. Add .d.ts files for eipw-lint-js.

Ricy137 commented 3 months ago

I'm interested in the issue, can I be assigned @SamWilsn ?

Also, please correct me if I got it wrong, the issue is due to the .d.ts file generated by wasm-pack isn't specific enough( filled with any type), so I need to update the automatically generated .d.ts file to more specifc types.

My question is currently the js package is generated automatically with wasm-pack and the js package as well as the targeted .d.ts file isn't in the repo currently (the repo has codes to generate the js package but not the final js package codes). So where shall I update the .d.ts file to? Thanks 🙏 .

SamWilsn commented 3 months ago

I'm interested in the issue, can I be assigned @SamWilsn ?

Of course!

Also, please correct me if I got it wrong, the issue is due to the .d.ts file generated by wasm-pack isn't specific enough( filled with any type)

Correct!

My question is currently the js package is generated automatically with wasm-pack and the js package as well as the targeted .d.ts file isn't in the repo currently (the repo has codes to generate the js package but not the final js package codes). So where shall I update the .d.ts file to?

It's a bit hairy (or at least it was the last time I looked.) I've done something similar over here, but it's a very manual process. The wasm-bindgen reference has a section on it.

This Stack Overflow answer has some options for doing it automatically, but I haven't investigated them much. ts-rs in particular seems weird in that it generates the bindings during cargo test. On the other hand, tsify doesn't seem as active of a project, but it works with wasm-bindgen.

Ricy137 commented 3 months ago

It's a bit hairy (or at least it was the last time I looked.) I've done something similar over here, but it's a very manual process. The wasm-bindgen reference has a section on it.

This Stack Overflow answer has some options for doing it automatically, but I haven't investigated them much. ts-rs in particular seems weird in that it generates the bindings during cargo test. On the other hand, tsify doesn't seem as active of a project, but it works with wasm-bindgen.

Thanks for your guides, they're very helpful! I've actually successfully implemented tsify and generated the intended .d.ts file. But this would involves modifications on eipw-lint, so I think manually adding the types generated by tsify would be a better approach. I've forked the repo and you can see my latest process here.

However, I failed to bind the types customarily defined with the lint and format function generated by wasm-bindgen. I got that I can use codes like here to use these customarily defined types in rust codes, but to combine these types in the lint and format functions require further modification for the related rust codes... This is where I'm blocked ( not knowing much about rust and doubt my inexperienced modifications would introduce unexpected negative effects).

Can you provide some help to break the blocks I met? And you can find all my codes here ( the master branch is where I want to manually add types, tsify/ts-rs branch is where I implemented tsify/ts-rs ). Thanks!

SamWilsn commented 3 months ago

Oh wow! You've gone way further down this road than I ever have.

But this would involves modifications on eipw-lint

I'm not opposed to modifications. How extensive are they? I tried to look through your diff, but I think you reformatted the code, so it's a bit hard to follow.

However, I failed to bind the types customarily defined with the lint and format function generated by wasm-bindgen.

Those might be easier to just do by hand. sources should be something like string[], options would be something like Opts | null, and the return value would be SnippetDef[] I think? wasm-bindgen takes care of the Option as far as I am aware.

Ricy137 commented 3 months ago

and the return value would be SnippetDef[] I think?

This is the tricky part I met. I can't just code as Result<Vec<SnippetJS>, JsError> or I would met the trait IntoJsResult is not implemented for Result<Vec<SnippetJS>, wasm_bindgen::JsError> error. TBH, I've been tearing my hair out trying to find a workaround or bypass to fix the error.

Also after changing the type declaration of lint and format, there would be type mismatch issues in test. Since OptsJS and SnippetJS are generated by wasm_bindgen, they didn't implement Deserialize. So I can't use from_value e.t. to convert the type, instead, I did the conversion manually here, the convert_to_js_object function is a helper functionto retaining original plain JavaScript object structure, and the convert_to_js_object is to convert Value type to OptsJS type.

How do you think of current codes. ?

Thanks 🙏 .

Ricy137 commented 3 weeks ago

Hey @SamWilsn. For type definition testing, I didn't find a way to use wasm-pack. I only find how to test functionality with wasm-pack but not for types checking. So I used Jest instead. Do you think it's acceptable?