core(tsc): NetworkRequest.RESOURCE_TYPES type fix #5851
Conversation
| @@ -20,6 +20,7 @@ const CHROME_EXTENSION_PROTOCOL = 'chrome-extension:'; | |||
| const compressionHeaders = ['content-encoding', 'x-original-content-encoding']; | |||
| const compressionTypes = ['gzip', 'br', 'deflate']; | |||
| const binaryMimeTypes = ['image', 'audio', 'video']; | |||
| /** @type {Array<string>} */ | |||
There was a problem hiding this comment.
so Array<LH.Crdp.Page.ResourceType> won't work anymore? Maybe I'm confused but I'm not sure why it's better now :)
What are the good cases we expect to catch by having Network.TYPES.Fetch be just 'Fetch' instead of LH.Crdp.Page.ResourceType?
There was a problem hiding this comment.
so Array<LH.Crdp.Page.ResourceType> won't work anymore?
uh, no, that will work and is way better :) fixing now.
The issue was that these are defined with const strings, so tsc wasn't widening them at all. e.g.
const textResourceTypes = [
NetworkRequest.TYPES.Document,
NetworkRequest.TYPES.Script,
NetworkRequest.TYPES.Stylesheet,
];was becoming only type Array<'Document'|'Script'|'Stylesheet'>, so textResourceTypes.includes() wasn't allowing you to pass it the general LH.Crdp.Page.ResourceType type (see microsoft/TypeScript#26255 for more on this annoyance :)
But Array<LH.Crdp.Page.ResourceType> is a way better fix than Array<string> for that.
There was a problem hiding this comment.
What are the good cases we expect to catch by having Network.TYPES.Fetch be just 'Fetch' instead of LH.Crdp.Page.ResourceType?
basically the inverse of the situation with the arrays. e.g if you had doSomething(resourceType: 'Fetch'|'XHR'), passing it NetworkRequest.TYPES.Fetch will be an error (since it's the full union). Not an issue in our current code, but it could be in the future.
(but it sounds like you're ok with this without the regression to Array<string> in the original form of the PR?)
After adding
SelfMapin #5849, went through all our uses ofRecord<>to see if we actually meant that the keys need to be mapped to themselves. This is the only case I could find.Unfortunately that meant that some other types needed to be widened to accommodate this, but good to have the type of
RESOURCE_TYPES.Fetchbe'Fetch'and not'XHR'|'Fetch'|'EventSource'|'Script'|...:)