solc-js icon indicating copy to clipboard operation
solc-js copied to clipboard

Migrating to Typescript

Open edisinovcic opened this issue 6 years ago • 7 comments

This PR is the migration of the whole library from javascript to typescript.

Jest testing framework is being used in rewrite instead of tape.

TODO:

  • check if the wrapper is okay in this format
  • fix coverage:jest problem with memory leakage (for now not part of the CI)

edisinovcic avatar Jan 21 '20 13:01 edisinovcic

Coverage Status

Coverage remained the same at 88.651% when pulling 83c1158af15a5b55d6a6995f51879712d2d0942b on blockchain-it-hr:ts-migrate into 036977ae1ab069504a6b4cff680d1ee12f70e760 on ethereum:master.

coveralls avatar Jan 21 '20 13:01 coveralls

@chriseth changes to .js files are due to semistandard, on the compiler.ts part all tests are running and it should be the same as compiler.js with types added.

It's a big change so please let me know if I missed something.

Also, I'm not sure if this is to be merged before all tests are rewritten to typescript so that coverage then can be tested properly.

edisinovcic avatar Jan 28 '20 19:01 edisinovcic

@axic please take a look at this when you have time. Tnx ;)

edisinovcic avatar Jan 29 '20 11:01 edisinovcic

@edisinovcic You probably should update the gitignore to include the .nyc_output/ path, since these should not be included within git.

stephen-slm avatar May 15 '20 13:05 stephen-slm

@edisinovcic You probably should update the gitignore to include the .nyc_output/ path, since these should not be included within git.

Tnx will do. I hope that I will have some time soon to get back to this and to finish it completely. Tnx for letting me know!

edisinovcic avatar May 15 '20 13:05 edisinovcic

Would love to see this one merged, let me know if there's anything I can do to help 😊

franco-roura avatar Sep 03 '21 16:09 franco-roura

@franco-roura I took a quick look at the PR and for me the biggest issue is that it looks pretty much like a complete rewrite of the library. Tests are being rewritten too so it's impossible to be sure if all the functionality is preserved. Reviewing this is going to be nightmarish and I suspect it would take more time than writing it :)

If you want to push this forward, then I think that the best course of action would be to find a way to do this piecemeal, in a set of smaller PRs. For example the switch to a different test framework could probably be done on its own, without modifying the code it tests. The PR adds some new dependencies - for example ESLint looks like something that can be added in a separate PR. Finally, since TypeScript is supposed to be backwards-compatible with JS, maybe we could first convert the project to TS with as few actual JS changes as possible and only after that add type annotations? This PR only adds new .ts files, without removing the .js equivalents so it's not even clear to me how much it modifies the original code.

Apart from the PR being big and unwieldy, you can probably see from the list of old, outdated PRs in this repo that we do not have enough manpower for reviewing these. The Solidity team consists mostly of C++ developers and most of the effort goes into the compiler. Any help in getting these PRs reviewed to the point where the biggest problems are ironed out would likely speed things up more than writing more code.

cameel avatar Sep 03 '21 19:09 cameel

Hi, thank you very much for the PR. But as pointed out, reviewing a PR that makes so many changes at once is not practical. However, this motivated others to contribute with small changes inspired by this PR, so thanks for your effort!

Part of your suggestions are already merged, and some are still in progress. If there are still things that you would like to add to solc-js, please feel free to open a new PR :D

r0qs avatar Sep 28 '22 21:09 r0qs