core(tsc): gatherer type-checking cleanup #5019
Conversation
| 'use strict'; | ||
|
|
||
| const Gatherer = require('./gatherer'); | ||
| const manifestParser = require('../../lib/manifest-parser'); | ||
| const Driver = require('../driver.js'); // eslint-disable-line no-unused-vars |
There was a problem hiding this comment.
@paulirish what's the story .js or not? :)
There was a problem hiding this comment.
i prefer without .js because that's what vscode autocompletes to but hey given the inconsistency right now, it doesn't matter until we fix it one way or the other. :)
| @@ -55,15 +55,16 @@ class StartUrl extends Gatherer { | |||
| return {isReadFailure: true, reason: manifest.value.start_url.debugString}; | |||
| } | |||
|
|
|||
| // @ts-ignore - TODO(bckenny): should actually be testing value above, not debugString | |||
There was a problem hiding this comment.
this is a bug we let slip in after the last rewrite. Having a debugString doesn't mean there's a failure necessarily. It could have been a warning but then fell back to the documentURL.
This means we're failing sites that use one of the fallbacks even though it's technically allowed. Above should be checking if there's not a manifest.value.start_url.value for the failure case, leaving here to pass on the value to try to load it (and probably passing along any debugString as a warning they messed up the relative URL or whatever).
along with #5018, add types (and descriptions) to all default artifacts
also adds types to
image-usage.jsandstart-url.js, which were more straightforward thanfonts.js(which is why that was split out and those aren't)this leaves
domstats.js,tags-blocking-first-paint.js, andfont-size.jswith@ts-nocheckinternally, but their output types and API are typed, so it's at least contained within those files. Maybe someday we'll get to them :)