Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: remove WebInspector.resourceTypes references #5556

Merged
merged 6 commits into from
Jun 27, 2018

Conversation

patrickhulce
Copy link
Collaborator

first step cleanup after NetworkRequest class, we only do string comparison in basically all cases except 2, those are easy to do as a set of strings, so no need for the complexity of the object.

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with some type discussion :)

Can we also delete third-party/devtools/ResourceType.js?

@@ -44,7 +44,7 @@ declare global {

_initiator: Crdp.Network.Initiator;
_timing?: Crdp.Network.ResourceTiming;
_resourceType?: ResourceType;
_resourceType?: Crdp.Page.ResourceType;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete import statement at top of file

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done


const SECURE_SCHEMES = ['data', 'https', 'wss', 'blob', 'chrome', 'chrome-extension', 'about'];

/** @type {Record<LH.Crdp.Page.ResourceType, LH.Crdp.Page.ResourceType>} */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, this could be tightened to

/** @type {{[ResourceType in LH.Crdp.Page.ResourceType]: ResourceType}} */

so the compiler would know the pairings of key to value (rather than just knowing keys and values are all LH.Crdp.Page.ResourceType, but not about how they are paired up).

But, somewhat ironically given our recent struggles with automatic type widening, this causes three other errors because use of this object does not get widened. So e.g. in critical-request-chains.js:

const nonCriticalResourceTypes = [
  NetworkRequest.TYPES.Image,
  NetworkRequest.TYPES.XHR,
  NetworkRequest.TYPES.Fetch,
  NetworkRequest.TYPES.EventSource,
];

becomes type Array<'Image' | 'XHR' | 'Fetch' | 'EventSource'>, and so
nonCriticalResourceTypes.includes(request._resourceType)
gives an error because LH.Crdp.Page.ResourceType isn't assignable to 'Image' | 'XHR' | 'Fetch' | 'EventSource'.

This is easily fixable by adding a /** @type {Array<LH.Crdp.Page.ResourceType> */ to the array (and to the Set query in uses-long-cache-ttl.js that also gives an error), but I'm not sure what the better trade off is here, less type precision or more manual-typing noise :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any situation right now in which the type precision you're proposing helps us?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any situation right now in which the type precision you're proposing helps us?

no, at least in reading during this review nothing leaped out at me as actually being helped by this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

then I'd vote keep it as it :)

@@ -46,7 +46,7 @@ class NetworkRequests extends Audit {
transferSize: record.transferSize,
statusCode: record.statusCode,
mimeType: record._mimeType,
resourceType: record._resourceType && record._resourceType._name,
resourceType: (record._resourceType || 'other').toLowerCase(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lowercase is confusing here, but I'm assuming this is to keep compatibility with old ResourceType.name...is it worth keeping it that way or should we just switch to the same capitalization as Page.ResourceType?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah this was for compat, but we're pre 3.0 official so I'm game to remove if others think it makes sense :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't have strong opinions, so consistency makes sense to me, but I haven't used this for anything. I guess the use cases are for

  1. programmatic consumption (maybe makes sense to match Page.ResourceType? But if I were creating the values for the first time myself I would never have capitalized the strings :)
  2. table display in the report. Which looks/reads better?

I don't really care :) @paulirish?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 doesn't really apply since network-requests audit is never displayed, just for programmatic consumption.

that leaves 1. I agree it makes sense to match Page.ResourceType and that if I were creating them myself I wouldn't have capitalized them, so... yeah @paulirish :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eh. let's nuke the toLowerCase. 🔥

@patrickhulce
Copy link
Collaborator Author

Can we also delete third-party/devtools/ResourceType.js?

whoops, yes I meant to do that :)

@patrickhulce patrickhulce merged commit 6e82902 into master Jun 27, 2018
@patrickhulce patrickhulce deleted the rename_web_inspector_requires branch June 27, 2018 20:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants