Typescript Base Support #566
Conversation
|
I only did a quick pass through the PR now, I will do a full review later. One thing though is that the PR is relatively big and that usually does not lend itself to quick and easy review :) It would be best if you could split it into smaller parts. Especially anything that can still be done on the JS version. Also the simple stylistic changes add a lot of noise in a bigger PR but on their own would be pretty easy to review. BTW, props for well structured and well described commits. That helps too! |
|
I agree, its mainly just the auto lint running causing that kind of pain. Its easily reverted and something that can be looked at after. I will remove the bloat. |
|
👍 on the suggestion to apply the style changes (var => const), linter changes, etc. in separate PRs prior to the TS PR. |
|
Any thoughts on these as of yet @cameel |
|
Sorry, I didn't have time to look at it yet, too many things going on in the main repo. I'll review it in the next few days. BTW, I was going to review #567 first but I see that it's not passing all CI tests. Is the PR done or are you still working on it? |
|
Can you maybe simplify the commits so that it does not go that much back-and-forth? |
|
@chriseth you want me to merge some commits ? |
Once you checkout the PR, you will get a better understanding on why its failing the check (V8 Node) and I wanted the conversion before I make changes to the pipeline files to remove failing CI test. |
Yes, IIUC, you have some commits that roll back others. |
Yeap no worries |
There was a problem hiding this comment.
I don't see any big problems, all of my comments are about small corrections.
As @chriseth has already mentioned, the number of commits should be reduced a bit. Overall the way you do commits is really good but we're only really interested in changes to the base branch so there should be no commits that just revert stuff you added/changed yourself in the PR. I mean, they're fine temporarily during the review but I think we're now at the stage where we'll be ready to merge it soon :)
There was a problem hiding this comment.
Looks great. There a few trivial things that would be nice to clean up but they're not something that would prevent the PR from being accepted so I'm approving already.
So as in #567, we still need an approve from @axic or @chriseth. You might also want to rebase this on #567 (not sure why you didn't just put it on top from the start - it will need to be on top of it eventually anyway).
I think you can also proceed with next steps in the TypeScript migration.
|
Oh, also coveralls job fails. Why does it see 0% coverage? It did pass before. |
Yeah I noticed, I will need to just resolve it. It cannot find the file for some reason and should not take much time. |
Coverage is passing and running which can be seen here: https://app.circleci.com/pipelines/github/ethereum/solc-js/840/workflows/84416478-f871-4c7a-91c6-20eef7dd1324/jobs/2830/steps I'm not sure why its not showing up. |
|
Honestly, I don't know much about how coverall is set up in this repo. Looks like the report comes from coveralls.io rather than CircleCI (the failed job on the list above had a link to https://coveralls.io/builds/44801275). If the job on CircleCI is supposed to submit data to coveralls.io then maybe it's some intermittent networking issue? There does not seem to be anything in the PR that would affect it so I'd try to retrigger the job and if that fails, just leave it until tomorrow and try again. If that does not help, we'll unfortunately need to look deeper into how it works and what specifically broke. |
If you haven't disabled "allow commits from maintainers", then I can push to your branch. Happy to help with rebasing as I'm causing a lot of that for you with merging other PRs. |
I would have to check that, but I have done the rebase. |
|
@axic i did not see anything regarding allowing maintainers but I have invited you to my fork, please rebase as much as you need. |
|
Github is misleading as it displays my avatar only, but the commit still has @stephensli as the author and me as the committer. The problem is likely that the author email address is not that of the account. @stephensli you may want to change the email addres on it, just rebase before merging so that my committer status is removed. Please note we'll merge #565 first so there will be one more rebase. |
smtchecker.ts
Outdated
| @@ -3,7 +3,7 @@ | |||
| // The function runs an SMT solver on each query and adjusts the input for | |||
| // another run. | |||
| // Returns null if no solving is requested. | |||
| function handleSMTQueries (inputJSON, outputJSON, solverFunction, solver) { | |||
| function handleSMTQueries (inputJSON: any, outputJSON: any, solverFunction?: any, solver?: any) { | |||
There was a problem hiding this comment.
@leonardoalt are all functions required or is solverFunction/solver still optional?
There was a problem hiding this comment.
I added the arguments as null where they're not needed, so I guess they could be mandatory, as long as null is acceptable
test/smtcallback.ts
Outdated
| @@ -25,7 +25,7 @@ function collectErrors (solOutput) { | |||
| return errors; | |||
| } | |||
|
|
|||
| function expectErrors (expectations, errors, ignoreCex) { | |||
| function expectErrors (expectations, errors, ignoreCex: any) { | |||
There was a problem hiding this comment.
@stephensli ignoreCex is now required, I guess we do not need the type for now?
There was a problem hiding this comment.
yeah its no longer needed, if its required and used then the any flag can be removed and or type defined.
|
Fixed some typing, it should be okay from my side, but we need to review these types/requirements closely before the next release. |
|
All though the changes now make the parts of the tests required, someone must go and resolve said tests. They should of been fixed in the changes merged but have been missed. Meaning this now fails. I can go and resolve said tests but I don't have enough domain knowledge to know if its right. I will have a look tomorrow. |
|
@stephensli I fixed it we just got into a force-push-war 😅 |
Yeah I messaged you on element regarding the war. |
|
I will stop modifying anything here now. Sorry for the confusion. |
| @@ -209,7 +210,7 @@ function setupMethods (soljson) { | |||
| } | |||
|
|
|||
| // Expects a Standard JSON I/O but supports old compilers | |||
| const compileStandardWrapper = function (input, readCallback) { | |||
| const compileStandardWrapper = function (input, readCallback?: any) { | |||
There was a problem hiding this comment.
I'm fine merging it assuming these types currently are only relevant intra-package and do not affect users of the library.
There was a problem hiding this comment.
i'm 99.9% sure are that these types are not exported for the consumer yet, e.g you can test with npm pack and see if any type files are outputted. I will just validate tomorrow.
Types are effectively all internal until we flag that they can be exported.
There was a problem hiding this comment.
At the moment everything is any type because solc is JS, not TS, and doesn't ship any definitions. A common strategy for migrating to TS is to make everything any just to get the build pipeline setup and then you can iteratively improve on the types as you go and it becomes easy for people to submit PRs
There was a problem hiding this comment.
^ this point is true but I was regarding about the type definition files which would be generated, since we have any and ? specified these type files can be generated and would not be empty.
if
// Generate d.ts files
"declaration": true,
was specified in the tsconfig then we could get files .d.ts files which can be included in the packaging process. Right now these don't exist (until we want them to be included).
Configuration reference for tsconfig can be located here: https://www.typescriptlang.org/tsconfig * Exclude the build output from source control by adding dist to .gitignore * Rename all project files from .js to .ts * Upgrade require to import and upgrade module.exports to export default * Refactored package.json to reference dist folder When building from typescript, the +x permission is missing, which is required to execute directly. The command line tool general usage and testing require it. The post build step adds this.
Base Typescript Support #287
This pull request includes the base requirements and minor changes to keep the current application code, test code, and process while supporting Typescript, Typescript compiling, and future support for adding types.
Files
All files have been moved from
.jsto.ts. This includes providing an extension type for the binary filesolcjswhich is nowsolc.tswhich will later be transpiled intosolc.js. The required package.json references have been updated where applicable to ensure packaging and binary referencing when linking work as expected.Linting
Linting has been directly upgraded from just
standard/standardto useeslintwhich is directly supported within Typescript, allowing code style enforcement ofstandard/standardwhile also locating and flagging possible code errors. Linting has been run throughout to changevartocontwhere applicable.Post Build Script
A new script file exists within the
buildfolder for post-build. This correctly sets the required permissions on thesolc.jscommand-line binary reference. Allowing testing to function correctly and direct execution to work as expected.Testing
Testing is done against the transpiled code after running through the Typescript compiler. This is due to the usage of dynamic importing being performed to pull in different binary versions. Each time this is performed the typescript compiler will attempt to parse and validate the entire file every single time. This increases testing time 10 fold and should be avoided. Testing against the built code allows testing to work as it does now.
When running the test command, additional resource files are copied into the
distfolder to allow testing to function correctly. This is due to Typescript not including those files in the build process.Packaging
When publishing or packaging, the dist files are used instead of the source typescript files as expected. You can test the content of the deployment or release by running the following
npm pack.What's next?
Once merged, this allows the introduction of typing to reduce errors and improve code quality. This can go in hand with enabling typescript declaration and configuration of the package.json to use said built types. Allowing users of the application to gain access to these types after installing the package.