-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Description
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-systemortestfiles. - Conversion to TypeScript
- Exporting type definitions
Current state
- Type checking: All
srcandextensionsfiles, except for the newly introducedsrc/corehave 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.jsfiles in build-system to be co-located with the files that need them. Currently onlysrc/core/\*\*/\*.extern.js(core externs)
- **"migrated externs" **- We’ve begun splitting the massive
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
pridetype-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/utilsandsrc/\*.jsflatfiles) - Works well for vertically-separated code with minimal external dependencies
/cc @samouri