♻️ 🏗 🚀 Begin adding type checking to the build-system directory.

[rileyajones]

By using typescript to compile the existing javascript we can catch a lot before they are introduced to the codebase without having to fully switch to typescript.

Unfortunately the existing code contains a significant number of errors so prior to adding this as a build step they there will need to be several pull requests filled with just typing changes.

For reference without this change there are 280 errors.
With this change there are 109
With this change and @types/mocha installed the number falls to 44.

Those 44 errors almost all actually errors that should be fixed and that we likely would not have found without this check.

#28387

[amp-owners-bot]

Hey @rsimha! These files were changed:

build-system/compile/compile.js
build-system/server/lazy-build.js

Hey @danielrozenberg! These files were changed:

build-system/compile/internal-version.js

Hey @alanorozco! These files were changed:

build-system/server/app-index/amphtml-helpers.js
build-system/server/app-index/file-list.js
build-system/server/app-index/html.js
build-system/server/app-index/template.js
build-system/server/app-index/test/helpers.js
build-system/server/app-index/test/test-amphtml-helpers.js
build-system/server/app-index/test/test-html.js

[rileyajones]

I am aware that this is a large change containing a lot of files, so, given that it is currently nonfunctional, I'm happy to split it apart further if anyone would prefer.

[rsimha]

Nice PR! I've added a few preliminary comments for the CI and task code. Can we split them off into one PR, and fix the server code in a separate PR? (They are fairly isolated from each other.)

[rsimha]

Curious: If this is being changed, would it make sense to change the signature as follows?

function exec(cmd, options = {'stdio': 'inherit'}) {

(Same question applies to other instances)

[rileyajones]

I think that would make sense in this instance, I'm not sure about all the others.

[estherkim]

Maybe @ types/node will help here too instead of defining a custom interface.

[rileyajones]

Unfortunately neither showStack nor status is not an attribute of Error so we either need this or a custom Error type.

[estherkim]

Ah I see.. looks like it's a gulp type. I don't know about status but I found showStack here https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/plugin-error/index.d.ts#L13

[rsimha]

Yep, showStack is for gulp, and should go away when we adopt a more lightweight task runner.

status is something we set in exec.js, so it might be possible to use a different mechanism:

amphtml/build-system/common/exec.js

Lines 80 to 97 in 29fdb58

	/**
	* Executes the provided command, piping the parent process' stderr, throwing
	* an error with the provided message the command fails, and returns the
	* process object.
	* @param {string} cmd
	* @param {string} msg
	* @return {!Object}
	*/
	function execOrThrow(cmd, msg) {
	const p = exec(cmd, {'stdio': ['inherit', 'inherit', 'pipe']});
	if (p.status && p.status != 0) {
	log(yellow('ERROR:'), msg);
	const error = new Error(p.stderr);
	error.status = p.status;
	throw error;
	}
	return p;
	}

[rileyajones]

I would rather not alter the existing error message handling system for now. Given that those are both properties that we do actually use this type extension makes sense to me, but should remove these properties once the code using them is gone.

[estherkim]

Ok that's fair, how about adding a TODO in global.d.ts to delete it

[rileyajones]

Done