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

[WASM] Converting runtime JS to TS #55871

Closed
wants to merge 23 commits into from

Conversation

Daniel-Genkin
Copy link
Contributor

@Daniel-Genkin Daniel-Genkin commented Jul 17, 2021

This PR renames the dotnet_support.js, binding_support.js and library_mono.js files to dotnet_support.ts, binding_support.ts and library_mono.ts and converts them to use typescript.

What's changed

Basically everything is as it was only now the files support typescript's type system. So we can better document the functions and have a better development experience as this also has the side effect of adding syntax checking and a few other linting features to the build and VS Code.

Most types can be included directly in the source code, however for some additional types, such as for C functions, there is a new types folder.

Also, now we can use enums to help minimize the guessing with what types are supported for what operations. They can be added to the declaration files via declare const enum. During compilation they will be automatically replace with their raw values.

The tsc compiler runs during the build but can be invoked manually via tsc -p <path to runtime folder> or via make js-from-ts from the MakeFile in the src/mono/wasm folder.

How does the new build work

The same as before except that now, before calling emcc to package the js into dotnet.js, we call the tsc compiler to convert the TS to JS. Then the emcc can run as before with the only difference being that the js files are located in a newly generated build folder.

TODO list

  • Convert JS to TS
  • Integrate into build
  • Add instructions to readme on setting up tsc (all you need is to run npm i -g typescript to install the tool globally)
  • Add tsc to CI if it isn't already there (it isn't: [dotnet\runtime WASM] Added typescript dotnet-buildtools-prereqs-docker#481)
  • Improve overall code quality
    • Type the parameters and global variables for each file
    • Replace hardcoded switch cases with enums for organization and readability
    • Get feedback on the types to make sure they are correct and clear

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@En3Tho
Copy link
Contributor

En3Tho commented Jul 17, 2021

Good work. Always nice to see the world becoming a better place by reducing js locs.

@terrajobst terrajobst added the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 19, 2021

@terrajobst I am an intern on @lewing 's team. So I am internal to MSFT :).

@terrajobst terrajobst removed the community-contribution Indicates that the PR has been added by a community member label Jul 19, 2021
@vargaz
Copy link
Contributor

vargaz commented Jul 20, 2021

Would it be possible to rename these to binding-support.js etc., our convention is to use '-' in file names instead of '_'.

@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 23, 2021

Reviewers please also review the types. I had to guess them so there are bound to be errors or better types available

I specifically left no implicit anys and tried to minimize the unknown and any types. However, for example, if I typed something as a number maybe we can make an alias type that is more useful such as ManagedPointer.

@Daniel-Genkin Daniel-Genkin marked this pull request as ready for review July 23, 2021 15:42
@thaystg thaystg requested a review from radekdoulik July 23, 2021 15:47
@thaystg thaystg added the arch-wasm WebAssembly architecture label Jul 23, 2021
@ghost
Copy link

ghost commented Jul 23, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

WORK IN PROGRESS

This PR renames the dotnet_support.js, binding_support.js and library_mono.js files to dotnet_support.ts, binding_support.ts and library_mono.ts and converts them to use typescript.

What's changed

Basically everything is as it was only now the files support typescript's type system. So we can better document the functions and have a better development experience as this also has the side effect of adding syntax checking and a few other linting features to the build and VS Code.

Most types can be included directly in the source code, however for some additional types, such as for C functions, there is a new types folder.

Also, now we can use enums to help minimize the guessing with what types are supported for what operations. They can be added to the declaration files via declare const enum. During compilation they will be automatically replace with their raw values.

The tsc compiler runs during the build but can be invoked manually via tsc -p <path to runtime folder> or via make js-from-ts from the MakeFile in the src/mono/wasm folder.

How does the new build work

The same as before except that now, before calling emcc to package the js into dotnet.js, we call the tsc compiler to convert the TS to JS. Then the emcc can run as before with the only difference being that the js files are located in a newly generated build folder.

TODO list

  • Convert JS to TS
  • Integrate into build
  • Add instructions to readme on setting up tsc (all you need is to run npm i -g typescript to install the tool globally)
  • Add tsc to CI if it isn't already there (it isn't: [dotnet\runtime WASM] Added typescript dotnet-buildtools-prereqs-docker#481)
  • Improve overall code quality
    • Type the parameters and global variables for each file
    • Replace hardcoded switch cases with enums for organization and readability
    • Get feedback on the types to make sure they are correct and clear
Author: Daniel-Genkin
Assignees: -
Labels:

arch-wasm

Milestone: -

@pavelsavara
Copy link
Member

We have at least 3 different things treated as number

  • naked pointer
  • GCHandle
  • JSHandle
    could we have separate TS types for those and have them not convertible to number ?
    Perhaps that could be follow-up PR.

@pavelsavara
Copy link
Member

It would be good to diff original js before and compiled js after to make sure that functional changes are reviewed.

@Daniel-Genkin
Copy link
Contributor Author

We have at least 3 different things treated as number

  • naked pointer
  • GCHandle
  • JSHandle
    could we have separate TS types for those and have them not convertible to number ?
    Perhaps that could be follow-up PR.

I am not familiar enough with these details so I agree the best way will probably be to leave them as number for this PR and then over time gradually improve them as we update the files.

@Daniel-Genkin
Copy link
Contributor Author

Daniel-Genkin commented Jul 26, 2021

It would be good to diff original js before and compiled js after to make sure that functional changes are reviewed.

I did a diff and looks like it is the same code just it uses spaces rather than tabs, some comments are intentionally removed and some spacing between functions, variables, etc. is optimized. However this is expected as tsc is copying the ts over to the js and then just slightly modifying it to remove the typescript types and I guess also running some basic linter. The underlying code however is not changed.

@steveisok
Copy link
Member

@lewing @pavelsavara Is there still ongoing work w/ this PR?

@pavelsavara
Copy link
Member

yes, I will take it over.

@pavelsavara pavelsavara self-assigned this Aug 31, 2021
@pavelsavara
Copy link
Member

Replaced by #59392

@ghost ghost locked as resolved and limited conversation to collaborators Nov 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-wasm WebAssembly architecture area-Build-mono
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants