馃彈 Rewrite and enable type-checking on src/core target #33921
Conversation
|
Hey @rsimha! These files were changed: Hey @jridgewell! These files were changed: |
| */ | ||
| const TYPE_CHECK_TARGETS = { | ||
| 'src': { | ||
| entryPoints: [ |
There was a problem hiding this comment.
Would it make more sense to use our bundle definitions as our typecheck targets instead of recreating a somewhat similar map? The primary entrypoints are in bundles.config.js and the extensions are in bundles.config.extensions.js.
In the short-term, I see how that would be difficult for enforcing src/core files, but in the long terms it seems more maintainable (and hopefully less code!).
It would also enable us to check-types for binaries in isolation (just one extension, or just v0.js, etc.)
There was a problem hiding this comment.
The short answer to this is: I don't know. To some degree, that would seem to make sense. When the whole repo is passing, we can enable the type--checking CC flag by default, and that will just get added into the dist process. But that's a bit "messier" to get working, as opposed to locking down type-safety one directory at a time. If we build this up so the whole repo is type-checked successfully, would there be any added value in type-checking the binaries themselves? (I'm guessing yes, but am not certain)
There was a problem hiding this comment.
Partial typechecking will not be valuable once everything is fixed up. The question is until we get there, what is more valuable? Fixing directory by directory where we have to code each in a one-off way? Or Binary by binary? I don't have that strong of an opinion since I figured adding in one-off directories is not that much work
There was a problem hiding this comment.
I get the sense that in practice, locking down one directory at a time (and including the check in CI) will make it easiest to take multiple bites at the apple and get the whole repo compliant, whereas we have to get a whole binary compliant to enable it during CI and for most of the binaries that entails most of src
* add compile options to not add anything unspecified * Rewrite check-types task to support checking individual targets * Update conformance allowlist for string helpers now in core * Define extern types for existing core files * Update core files to pass type-checking * %s/object.externs.js/object.extern.js/ * Remove duplicate externs defined in core * Opt-out failing targets from type-checking; let flag override * Clean up extern list code * Lint fixes * destructure * typo * comments
Related to #33631
Example usage:
amp check-types --targets=src-core;amp check-types --warning_level=verboseCurrently, type-checking the entire source tree fails with various errors and warnings. It pulls in almost every source file in the repo, then trims it down based on what's required by the binary. This makes sense for compilation, but isn't necessary for type-checking.
This PR allows for type-checking a specific directory--in this case,
src/core. Externs belonging tosrc/coreare defined there, rather than inbuild-system/amp.externs.jswhich has grown to be huge (~1100 lines last I checked). By default,amp check-typesdoesn't run at all on targets with awarning_level=QUIET, since that effectively does nothing but take time. The only enabled target issrc/corefor now, but this will be steadily expanded./cc @ampproject/wg-performance