Skip to content

refactor(typescript): refactor to ts (fixes #56) #100

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

Merged
merged 6 commits into from
Sep 29, 2021
Merged

refactor(typescript): refactor to ts (fixes #56) #100

merged 6 commits into from
Sep 29, 2021

Conversation

mathe42
Copy link
Contributor

@mathe42 mathe42 commented Sep 25, 2021

I wanted a look at the complete libary and took the chance to rewrite it to TS.

Can be optimized (for example TS-Strict mode is a good idea). But this results in a lot of errors.

@mathe42 mathe42 mentioned this pull request Sep 27, 2021
Copy link
Owner

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woah! Thank you SOOOOOO much for doing this porting work!

I'll also cc @surma again here, since it touches a lot of their code they implemented. But! I think you and I can handle this for the most part.

If you don't mind me asking, after doing the port, have you tried using the library from a non TS app? Like do all the same quick start examples and things work? 😄 I can test that if you would like!

Let me know when you feel this PR is ready, I can do some testing on my end, and I think we can start considering merging this! Again, thank you SOOOOO much for doing this! 😄 🎉

export function getAscToJsConverterForType(typeName) {
return getConverterForType(typeName)?.ascToJs;
export function getAscToJsConverterForType(typeName: string) {
return getConverterForType(typeName).ascToJs;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure that the typescript typings will remove the need for the ? 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was not needed before as well as getConverterForType always returns a non nullish value.

@mathe42
Copy link
Contributor Author

mathe42 commented Sep 27, 2021

If you don't mind me asking, after doing the port, have you tried using the library from a non TS app? Like do all the same quick start examples and things work? 😄 I can test that if you would like!

Actualy I did not use it in a TS app 😉 only a simple JS

Only type missmatch I could think of is in

as-bind/lib/types.ts

Lines 3 to 7 in 3ff4b1d

export type WebAssemblyModuleStreaming =
| WebAssemblyModuleSync
| Response
| PromiseLike<Response | WebAssemblyModuleSync>;
export type WebAssemblyModuleSync = BufferSource;
I'm not shure if I missed some types there. (used in lib.ts)

@mathe42
Copy link
Contributor Author

mathe42 commented Sep 28, 2021

Let me know when you feel this PR is ready, I can do some testing on my end, and I think we can start considering merging this! Again, thank you SOOOOO much for doing this! 😄 🎉

Ready for review.

@torch2424
Copy link
Owner

This was not needed before as well as getConverterForType always returns a non nullish value.

Sounds good to me! 😄

Actualy I did not use it in a TS app wink only a simple JS

Yay! Awesome, thank you! 😄 🎉

Ready for review.

Awesome! So since there wasn't any changes since the last time I looked at the code, I am going to approve and merge! 😄

Copy link
Owner

@torch2424 torch2424 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also went ahead and tested this in VSCode, and it worked great! 😄 🎉

Screenshot from 2021-09-29 10-19-43

@torch2424 torch2424 merged commit 0e3a585 into torch2424:master Sep 29, 2021
@torch2424
Copy link
Owner

@mathe42 I am going to merge in all your changes, and then cut a minor release after I review and merge all of your code 😄

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.

2 participants