Skip to content

I2I: Restoring AMP’s Type Checking #34096

@rcebulko

Description

@rcebulko

Overview

For the past year, type checking has been accidentally disabled for the amphtml repository. Unfortunately, flipping the switch back on is not so simple because during this time period a very large number (1000s) of type errors have organically been created. Restoring type-checking would help avoid bugs and potentially allow for more advanced compilation optimizations

This document proposes a multi-pronged approach for gradually restoring type checking to AMP binaries.

Goals

  • All JS files that end up in AMP binaries are type checked as part of CI.
  • Organize AMP’s extern files and type definitions

Non-goals

  • Type-checking build-system or test files.
  • Conversion to TypeScript
  • Exporting type definitions

Current state

  • Type checking: All src and extensions files, except for the newly introduced src/core have type checking disabled.
  • **Externs: **All of AMP’s externs live within 5 files in build-system/externs, and there are 100s of them. This has similar issues with having just a few giant CSS files. It is hard to track which source files depend on which externs and therefore hard to know when it is ok to delete one.
    • **"migrated externs" **- We’ve begun splitting the massive *.extern.js files in build-system to be co-located with the files that need them. Currently only src/core/\*\*/\*.extern.js (core externs)

Approaches

At a high level, there are two kinds of approaches we’d like to pursue: vertical and horizontal.

  • Vertical means establishing a set of files that fully pass all type-checking rules and incrementally increasing the set of files in that set until everything is included.
  • Horizontal means establishing specific type-checking rules that all files must pass, and then gradually increasing the set of rules until all rules are included.

Horizontal: Gradually increase the minimum type quality via crowdsourcing

Goal: Establish a floor of type quality that each and every AMP Developer is required to maintain.

Source files: --local_changes in each branch.

Encourage (force) developers to ensure the types within the files they are modifying have a baseline of quality. We do this by failing CI when specific JSC errors are found in the modified files. We can also gradually increase the failable error types as time goes on. The first set of errors we should include are

  • JSC_TYPE_PARSE_ERROR - Types should at least parse properly.
  • JSC_INVALID_PARAM - Parameter types should parse.

Top-down: expand passing targets

Goal: Lock down passing file(s), preventing new type errors from being introduced to them

**Source files: **any

Externs: migrated externs and build-system externs

  • Single catch-all pride type-check target enabling CI type-checking
  • Works well for files that could pass type-checking but haven't yet been sorted/organized via bottom-up approach into a folder that is fully passing

Bottom-up: target top-level src directories

Goal: Establish and lock-down type-safe directories

Source files: subdirectories of src (ex. context, polyfills, purifier, etc)

**Externs: **its own migrated externs along with core externs

  • Minimal surface of external dependencies can be provided via shame.extern.js
    • This "shame" file contains externs that exist only during migration, and act as a to-do list of types that should be condensed or organized elsewhere
    • This also acts to unblock bug-fixes when a type is needed but not yet defined in available externs
  • As each is made to pass, enable it in CI
  • As files move to core, they'll be updated to pass (much of src/utils and src/\*.js flatfiles)
  • Works well for vertically-separated code with minimal external dependencies

/cc @samouri

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions