core(tsc): add type checking to seo gatherers #4991
Conversation
| return {href, hreflang}; | ||
| return { | ||
| href: href || '', | ||
| hreflang: hreflang || '', |
There was a problem hiding this comment.
@kdzwinel the compiler complained that these could be null, not just strings. It is correct wrt getAttribute, and I guess there could be some malformed <link>s in pages? However the audit was expecting only strings here, so I'm not sure if I'm missing something or we just aren't covering that possibility yet
There was a problem hiding this comment.
It looks like you found a bug (this whole typescript thing might be a good idea after all! 😉) . Hreflang audit is certainly not prepared for hreflang being null (https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/audits/seo/hreflang.js#L34).
There was a problem hiding this comment.
are you ok with leaving with these defaults for now and you could update to what makes sense when you get the chance? (I assume handle the null case if matching the pattern of the other seo artifacts)
There was a problem hiding this comment.
are you ok with leaving with these defaults for now
yeah, that SG
| /** | ||
| * @param {!Node} node | ||
| * @param {LH.Artifacts.FontSize.DomNodeWithParent} node |
There was a problem hiding this comment.
should we have any guidance on when to break out into separate interface vs. do it inline and the LH.Artifacts['Artifact'] method?
There was a problem hiding this comment.
I'm open to whatever. We could always access by property (even if there's a distinct interface) just to keep it consistent. Or we could always define a distinct interface, even if the type is string|null (really the only downside is excessive file size, but d.ts files always end up huge).
The rule I was following was strictly "can it fit easily on one line" and had no other significant thought put into it :)
| @@ -3,6 +3,7 @@ | |||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | |||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | |||
| */ | |||
| // @ts-nocheck | |||
There was a problem hiding this comment.
yeah. The file reaches into WebInspector.CSSMatchedStyles and mutates types in filter/map pipelines which always ends up as a lot of work in TS. I got pretty far but eventually gave up, so settled just for param/return types for now :)
| .then(content => ({status: response.status, content})); | ||
| }) | ||
| .catch(_ => ({status: null, content: null})); | ||
| return response.text().then(content => ({status: response.status, content})); |
There was a problem hiding this comment.
should we just await this too while we're at it? I think we might have to for the semantics to be the same anyhow
except font-size
|
appveyor is refusing to pass, but it's just the byte-efficiency smoke tests (and, to its credit, |
except only some basic fixes in font-size for now, because it really likes mutating types and it calls into WebInspector stuff :P
No functional changes.