Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Provide type support to wrapper.ts #615

Closed
wants to merge 1 commit into from
Closed

Provide type support to wrapper.ts #615

wants to merge 1 commit into from

Conversation

stephen-slm
Copy link
Contributor

@stephen-slm stephen-slm commented Apr 3, 2022

Why

This change provides some basic type support for the internal and exported methods within wrapper.ts. The target result of this PR is to increase readability, maintainability and start the introduction of types. This additionally exports types out to the consumer of the solc module for consuming by the user. These are the following:

Depends on #614

Note

This is currently rebased on #614 so just validate the latest commit.

@coveralls
Copy link

coveralls commented Apr 3, 2022

Coverage Status

Coverage remained the same at 84.537% when pulling 85dd58d on stephensli:type/wrapper into 05a1955 on ethereum:master.

@stephen-slm stephen-slm marked this pull request as ready for review April 4, 2022 11:44
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Apr 8, 2022
@cameel cameel requested a review from r0qs September 19, 2022 15:51
@cameel cameel added refactor and removed has dependencies The PR depends on other PRs that must be merged first labels Sep 20, 2022
Copy link
Member

@r0qs r0qs left a comment

Choose a reason for hiding this comment

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

Hey, @stephensli thank you very much for this amazing work! :)
I took a look at your PR and made some comments. Some comments require discussion with the rest of the team to decide on the best directions. But I would appreciate your input on them as well.

@@ -24,3 +24,347 @@ export interface LibraryAddresses {
export interface LinkReferences {
[libraryLabel: string]: Array<{ start: number, length: number }>;
}

export interface SolJson {
Copy link
Member

@r0qs r0qs Sep 21, 2022

Choose a reason for hiding this comment

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

I know that this PR did not introduce this, but since we are doing some refactoring already, maybe we could move the types to a more proper directory. I don't particularly appreciate that the helpers.ts and types.ts share the same common directory.

We don't need to do it in this PR, but maybe it is not that much effort to do it now. And I would appreciate some suggestions on this.

I think we could have a types folder under the root, or maybe src/types, with the type definitions and move the source to a src directory.
Then in the types folder, we can add a index.ts with the types we want to export because we may not want to export everything to the consumer.
I believe this would require changing the tsconfig.json to something like:

 "rootDirs": ["src"],
 "typeRoots": ["./src/types", "node_modules/@types"]

The type declarations should follow the same directory layout of the source. For instance, bindings types would be under types/bindings.

Also, everything (source and types) is currently emitted together under the dist. And since we are adding support to types, I think it is a good opportunity to turn off "declaration" in the tsconfig.json and build the types separately, like so: npx tsc --emitDeclarationOnly --declaration --outDir dist/types

We can also add it as a script in the package.json.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @stephensli, we decided that this can be considered out of the scope of this PR. We can make this change later. So please ignore this comment here for now. I hope you didn't spent time on it :)

Let me know if you need any help to finish this PR so we can get it merged :D

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, i'll try and find some time during the week to complete the remainder otherwise it will be completed on the weekend.

bindings/core.ts Outdated
copyFromCString: unboundCopyFromCString.bind(this, solJson),
copyToCString: unboundCopyToCString.bind(this, solJson, core.alloc),

// @ts-ignore
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

*
* @solidityMaxVersion 0.5.0
*
* @param input
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should expand in the description of these parameters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah happy to update, what would you like these to be described as for any consumer to understand?

Copy link
Member

@r0qs r0qs Sep 26, 2022

Choose a reason for hiding this comment

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

Sure, for instance:

/**
 * Compile a single file.
 *
 * @solidityMaxVersion 0.5.0
 *
 * @param input The JSON formatted compiler standard input
 * @param optimize A flag for enable/disable compiler optimization
 */
export type CompileJson = (input: string, optimize: boolean) => string;

// Allow JS must be included to ensure that the built binary is included
// in the output. This could be copied directly in the future if required.
"allowJs": true,
// TODO:
// In order to gracefully move our project to TypeScript without having
// TS immediately yell at you, we'll disable strict mode for now.
"strict": false,
"noImplicitAny": false
"noImplicitAny": false,
"declaration": true
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can also add:

    "noEmitOnError": true,
    "noImplicitReturns": true,
    "noUnusedLocals": true,
    "noUnusedParameters": true

Copy link
Member

Choose a reason for hiding this comment

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

And if we decide to emit the type declarations separated from the source explicitly, we can set the declaration to false here.

@stephen-slm
Copy link
Contributor Author

Hey, @stephensli thank you very much for this amazing work! :) I took a look at your PR and made some comments. Some comments require discussion with the rest of the team to decide on the best directions. But I would appreciate your input on them as well.

@r0qs Thank you for reviewing, anything to do with code changes are entirely out of scope of this PR as this is purely regarding the type information. It does not mean it's not valid but we should try keep it focused until this is complete.

I will go through and still reply to everything.

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
No open projects
Status: Review
Development

Successfully merging this pull request may close these issues.

4 participants