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

Adds TypeScript definition for the package's supported public surface area. #298

Closed
wants to merge 4 commits into from

Conversation

MicahZoltu
Copy link
Contributor

@MicahZoltu MicahZoltu commented Nov 19, 2018

Note: While this function takes and returns a string, the string is really JSON with a very specific and well defined structure. A future PR will (hopefully) add the definitions for those JSON objects. This PR is meant to be an agreeable starting point for getting TypeScript definitions included.

Split off #205.

@coveralls
Copy link

coveralls commented Nov 19, 2018

Coverage Status

Coverage remained the same at 83.85% when pulling 31b5403 on MicahZoltu:micah into 5c1280c on ethereum:master.

@axic
Copy link
Member

axic commented Dec 3, 2018

Thanks!

Can you also add the other functions: https://github.com/ethereum/solc-js/blob/master/wrapper.js#L216-L238 ?

Also not sure how to deal with the other public entry points in abi, linker and translate. All of them can be used independently (see the README).

@MicahZoltu
Copy link
Contributor Author

I would prefer we merge this as is, so people have a starting point against which they can submit PRs for additional definitions.

The original issue has been open for 9 months now, and this PR doesn't contain any of the content that I want it to contain, so you hopefully can appreciate my apprehension to continuing to do more work in this PR when it feels like no progress is being made on getting anything merged and the stuff I wanted to merge (type definitions for the compiler output JSON) has been pulled out just so we can merge something.

@axic
Copy link
Member

axic commented Dec 3, 2018

I understand, though I am not convinced how the JSON definition should be included here given those functions only take strings.

Ok, I'll add some of these myself then, but I cannot validate what I add is correct. compile on its own is not enough since every user of this API uses at least version too.

@axic
Copy link
Member

axic commented Dec 3, 2018

Added some of the missing definitions.

@MicahZoltu how can we run tests on this?

@MicahZoltu
Copy link
Contributor Author

You can't really "test" type definitions in an automated way. It is like writing "tests" for documentation. You can make it so the documentation is auto-generated, just as you can make it so the definitions are auto-generated (by writing in TypeScript) but "testing" doesn't really make sense in either context.

@axic
Copy link
Member

axic commented Dec 3, 2018

You can't really "test" type definitions in an automated way.

I want to write an actual test case.

@MicahZoltu
Copy link
Contributor Author

MicahZoltu commented Dec 3, 2018

Oh, do you mean you want to try to write a little TypeScript script that uses the definition? Here is a script I have that uses version 0.4.25 of the compiler and has type definitions manually added to the project (what lead me to submitting the original PR so others didn't have to repeat the manual process like I did):

public async compileContracts(): Promise<CompilerOutput> {
	const compilerInputJson = await generateCompilerInput()
	const compilerOutputJson = compileStandardWrapper(JSON.stringify(compilerInputJson))
	const compilerOutput: CompilerOutput = JSON.parse(compilerOutputJson);
	const errors = (compilerOutput.errors || []).filter(error => !/Experimental features are turned on\. Do not use experimental features on live deployments\./.test(error.message))
	if (errors) {
		let concatenatedErrors = "";

		for (let error of errors) {
			concatenatedErrors += error.formattedMessage + "\n";
		}

		if (concatenatedErrors.length > 0) {
			throw new Error("The following errors/warnings were returned by solc:\n\n" + concatenatedErrors);
		}
	}

	return compilerOutput
}

public async generateCompilerInput(): Promise<CompilerInput> {
	const sourceRoot = path.join(__dirname, '../../source/')
	const filePaths = await recursiveReadDir(sourceRoot)
	const filesPromises = filePaths.map(async filePath => (await readFile(filePath)).toString('utf8'))
	const files = await Promise.all(filesPromises)

	const inputJson: CompilerInput = {
		language: "Solidity",
		settings: {
			optimizer: {
				enabled: true,
				runs: 500
			},
			outputSelection: {
				"*": {
					"*": [ "abi", "evm.bytecode.object" ]
				}
			}
		},
		sources: {}
	}
	for (var file in files) {
		const filePath = filePaths[file].replace(sourceRoot, "").replace(/\\/g, "/")
		inputJson.sources[filePath] = { content : files[file] }
	}

	return inputJson
}

You could trim out the error processing if you wanted to trim it down a bit, and hard-code a compiler input file rather than looking at files in a particular directory.


As a side note, this also shows why it is useful to have the JSON input/output as type definitions. CompilerInput and CompilerOutput in this example are type definitions that match what the compiler wants and what it returns. While the function technically returns a string, I get full type checking on both the input (which warns me if I supply something that I shouldn't or am missing a required field) and the output (which warns me if I try to access a field that doesn't exist and it also gives me auto-complete for discovery during authoring).

IMO, compileStandardWrapper should be taking in a JavaScript object and returning a JavaScript object, not a JSON string. JSON strings are meant to be a serialization format, not a format for passing data around within a program. In this case, when I call compileStandardWrapper, I am passing it configuration and getting back a complex object. I understand that the underlying compiler executable is using JSON as a serialization format, but the JavaScript wrapper (solc-js) should (IMO) be accepting an object and returning an object, since that is really what I want and unlike calling a binary, we can work with well defined complex objects in JS and not just strings.

@axic
Copy link
Member

axic commented Dec 3, 2018

IMO, compileStandardWrapper should be taking in a JavaScript object and returning a JavaScript object, not a JSON string.

I know you have been mentioning this several times, but doing it as comments on PRs will not lead anywhere. Please open an issue to have it properly discussed. I am not against it, but it is a breaking change and it has to wait at least til 0.6.0.

@MicahZoltu
Copy link
Contributor Author

I just noticed this has an "in-progress" tag on it. Is this waiting on me for something? Last message asked for a test case, but that doesn't really make sense for TypeScript definitions. If this is still something that you desire, can you please provide more details on what you mean by that?

@MicahZoltu
Copy link
Contributor Author

Reading back over some of the previous comments, it should be noted that TypeScript definitions are valuable without being complete. Having a TS definition for the compile function is useful to users even if there are other functions that don't have definitions. The compiler will simply treat the functions without definitions as any which is the same as if there was no definition file at all.

@Legogris
Copy link

Legogris commented Jun 8, 2019

@MicahZoltu Just found this and read the conversation. Why don't you split off an @types/solc package? This is something many package maintainers do, and there are also those maintained by random users.

@MicahZoltu
Copy link
Contributor Author

The advice from the TypeScript team, which I agree with, is to prefer to bundle the definition files into the original package if at all possible. The general recommendation is to submit a PR to the maintainers of the JS package with definitions, which is what I have done here.

Of course, it appears that this isn't going to ever get merged, so the fallback option of a PR to definitely typed is probably the path to take at this point. If someone else wants to do so, feel free to copy the contents of this PR (or the previous one) if you find it valuable. Next time I'm working with solc I'll probably do the same myself, though I don't know when exactly that will be.

@Legogris
Copy link

@MicahZoltu Sure, I might do just that, Just thought I'd suggest it before I take your code elsewhere :)

BTW I fully agree that it would be much preferable to have it merged in this repo here and I think it's a shame it got pushed forward this much away - minimal coverage as a starting point provides something for others to build on is strictly better than no interfaces at all IMO. Very few packages have complete and completely correct mappings and it's perfectly idiomatically fine to start off with any for a lot of fields and parameters.
@axic Do you still see any blockers for this..?

@axic
Copy link
Member

axic commented Jul 4, 2019

I think the ask to have a single test written in typescript using the definitions makes sense, because otherwise we have no way telling if the interfaces are valid, since none of the maintainers are fluent in typescript.

I'd more than happy to merge this with a single test included.

The mid term goal however is #287.

@MicahZoltu
Copy link
Contributor Author

I am a big fan of converting the project to TypeScript. Realistically, it is probably almost as easy to convert the project to TypeScript as it is to create "one test written in TypeScript" since for many projects, the majority of the work in converting to TypeScript lies in getting the build infrastructure all setup. Once that is done, it is just a matter of typing a bunch to fill in type information, which for a project of this size should go pretty fast.

That being said, I have other things on my plate so for now I'm hoping someone else takes up the torch on that matter. Whoever it is, feel free to use any of the type information I have in this PR on the other!

@r0qs
Copy link
Member

r0qs commented Sep 28, 2022

Hi, thanks for your contribution. I will close this PR since #615 already exported the type definitions of the wrapper. About the type definitions for the compiler JSON input/output there is already a work in progress here: #630.

Also, change from JSON strings to objects is, indeed, something that could be done, but it requires a bit more discussion and will not be in this PR anyway.

@r0qs r0qs closed this Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants