core(tsc): add type checking to many core gatherers #4975
Conversation
brendankenny
force-pushed
the
tsgatherers
branch
from
d96df66
to
706fe21
Compare
brendankenny
force-pushed
the
tsgatherers
branch
from
706fe21
to
d5ac17f
Compare
| } | ||
| /** | ||
| * @param {string} pageUrl | ||
| * @param {Driver} driver |
There was a problem hiding this comment.
return type is inferred? sweet!
| if (this._onRequestIntercepted) { | ||
| driver.off('Network.requestIntercepted', this._onRequestIntercepted); | ||
| } | ||
| await driver.sendCommand('Network.setCacheDisabled', {cacheDisabled: false}); |
There was a problem hiding this comment.
heh, do we think this had any real impact on results so far then? probably didn't matter I suppose since the page was already loaded
There was a problem hiding this comment.
yeah, technically it was running, just messing up the promise chain :) In theory some kind of racy thing could happen (or if there was an error, you'd end up with an unhandled promise rejection), but since we don't run this in smoke testing we'll never know! :)
| function parseThemeColor(jsonInput) { | ||
| return parseColor(jsonInput.theme_color); | ||
| } | ||
|
|
||
| /** | ||
| * @param {*} jsonInput |
There was a problem hiding this comment.
should we be adding todos around any/* to type them later? speaking of which are we standardizing on any or * I'd have a slight preference for any thinking about it now just from a searching perspective
There was a problem hiding this comment.
should we be adding todos around any/* to type them later
I think for most cases, yes. There's usually no reason not to type stuff, so I think it would be good to go back and hit all our marks.
In this particular case, though, I'm not sure what the best thing is to do. We know what type we expect on each property, but the point of this file is taking some json to parse it and flag anything we don't expect :) Technically the type should be anything serializable by JSON, so we could narrow that, at least...
yeah, sorry, I'll keep things smaller in the future |
Turned on type checking for the top-level
gatherers/, but then// @ts-nochecked three of the more involved ones :)Everything here should be a non-functional change. Promise -> aync/await stuff is easier to view in ?w=1.
The only functional change was to mixed-content.js, which had a few issues (promise passed to
promise.then(), event handler not actually removed), and it's not really observable except for fixed bugs and to the unit test which was reaching inside.