core(tsc): add type checking to fonts gatherer #5018
Conversation
| [ | ||
| driver.evaluateAsync(`(${getAllLoadedFonts.toString()})(${args})`), | ||
| driver.evaluateAsync(`(${getFontFaceFromStylesheets.toString()})()`), | ||
| ] | ||
| ).then(([loadedFonts, fontFaces]) => { | ||
| return loadedFonts.map(fontFace => { | ||
| if (fontFace.err) { |
There was a problem hiding this comment.
as far as I can tell, this was a bug. fontFace came from getAllLoadedFonts while it was getFontFaceFromStylesheets that generates the error objects it's looking for
| stretch: rule.style.fontStretch || 'normal', | ||
| style: rule.style.fontStyle || 'normal', | ||
| weight: rule.style.fontWeight || 'normal', | ||
| variant: rule.style.fontVariant || 'normal', | ||
| // @ts-ignore (currently) non-standard Chrome extension to CSSStyleDeclaration |
There was a problem hiding this comment.
i dont think these are non-standard, they're just fairly new.
There was a problem hiding this comment.
i dont think these are non-standard, they're just fairly new.
apparently (supposedly?) the non-standard part is taking standard @font-face descriptors and putting them as properties on CSSStyleDeclaration.
This is based on discussion in https://bugzilla.mozilla.org/show_bug.cgi?id=1296373#c4, which is admittedly 2 years old, but the linked draft specs seem to still back this up
There was a problem hiding this comment.
adding link to that discussion
| } | ||
| } | ||
| // Flatten results | ||
| return Promise.all(promises).then(fontFaces => [].concat(...fontFaces)); | ||
| return Promise.all(corsDataPromises).then(corsFontFaces => data.concat(...corsFontFaces)); |
There was a problem hiding this comment.
nice clean up separating promises and data!
typings/externs.d.ts
Outdated
| @@ -15,6 +15,13 @@ declare global { | |||
| code?: string; | |||
| } | |||
|
|
|||
| /** Make properties K in T optional. */ | |||
| type MakeOptional<T, K extends keyof T> = { | |||
There was a problem hiding this comment.
Does this actually get used anywhere? Seems like you might have opted for Exclude instead
There was a problem hiding this comment.
Does this actually get used anywhere?
yeah, bad split from #5019
one of the big @ts-nochecked gatherers