Amphtml: Add JSDoc enforcement + some sort of type checking for `build-system/`

Created on 13 May 2020  路  9Comments  路  Source: ampproject/amphtml

For the AMP runtime, we enforce JSDoc requirements for all code, and check types using closure compiler. However, build-system/ currently lacks JSDoc enforcement, and we do not type-check it using closure.

This issue is to track two things:

  • [ ] Enforce JSDoc requirements and backfill missing documentation for existing code.
  • [ ] Do some sort of type checking. If not closure, perhaps use typescript? Or failing that, maybe there's an eslint plugin that can do it?
When Possible Bug infra

Most helpful comment

What if we wrote build-system in TypeScript?

All 9 comments

/cc @ampproject/wg-infra and @ampproject/wg-runtime for opinions / ideas.

What if we wrote build-system in TypeScript?

Typescript would be cool, and you won't need to spin up a java process to typecheck. The Typescript language server can be spun up by your editor by default, and run the checks incrementally as you code.

Agree build system is a good target for Typescript. Explicitly defining some of the types might also make obvious some ways to organize existing infra

What if we wrote build-system in TypeScript?

I agree with the long-term goal of moving to TS. However, I suspect it will be a multi-quarter effort to rewrite the 40K+ lines of JS code in build-system/. Not sure if / when we'll have the bandwidth to prioritize something like this. (Or do you think I'm overestimating the difficulty of this job? 馃槂)

Meanwhile, I think it's worth enforcing and backfilling JSDoc annotations, which should take substantially less time than a full rewrite, and help avoid simple coding bugs.

Also, the link I shared in the issue description seems to suggest that it's possible to directly type-check JSDoc in .js files by invoking typescript with the --checkJs flag. If this actually works, it too will be substantially easier than a full rewrite.

I agree with the long-term goal of moving to TS. However, I suspect it will be a multi-quarter effort to rewrite the 40K+ lines of JS code in build-system/. Not sure if / when we'll have the bandwidth to prioritize something like this. (Or do you think I'm overestimating the difficulty of this job? 馃槂)

I don't think a full rewrite is needed before we can start using TS, we can adopt it incrementally. Some possible ways include:

  1. Converting files to .ts piecemeal instead of all at once. This allows us to have a relatively more strict tsconfig.
  2. Converting them all at once but having an initially lax tsconfig that we gradually make more strict (e.g. with any's).

Dusting off this old issue and assigning to our new Infra friend @rileyajones, who mentioned wanting to do something like this today 馃槂

I'll echo @samouri above and +1 the suggestion to make all of build-system run TypeScript all at once, but just spit out warnings until tasks can gradually be migrated

Converting files to .ts piecemeal instead of all at once. This allows us to have a relatively more strict tsconfig.

I would instead suggest using JS syntax only with Typescript JSDoc annotations. 0 runtime dependencies to run the code, which means running gulp (which is already extremely slow for us) won't get any slower.

Was this page helpful?
0 / 5 - 0 ratings