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

misc(typings): convert hardcoded strings to networkRequests.TYPES #5674

Merged
merged 2 commits into from
Jul 17, 2018

Conversation

wardpeet
Copy link
Collaborator

Summary
Just fixing some changes we missed for resourceType

Related Issues/PRs

@wardpeet
Copy link
Collaborator Author

wardpeet commented Jul 17, 2018

I stumbled upon some other hardcoded strings I'm not sure if they are intentional or not so just checking if I need to change them too.

estimateTransferSize in the byte-efficiency-audits we call resourceType as a string not sure if we should move to NetworkRecord.TYPES too.

which means
const totalBytes = ByteEfficiencyAudit.estimateTransferSize(networkRecord, contentLength, 'Script');
becomes

const totalBytes = ByteEfficiencyAudit.estimateTransferSize(networkRecord, contentLength, NetworkRequest.TYPES.Script');

brendankenny
brendankenny previously approved these changes Jul 17, 2018
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.

Since they're equivalent (the enum value is just that string) and both approaches (enum vs raw string) are now correctly type-checked, I personally don't really care which is used.

Sometimes the enum seems way too verbose, sometimes it adds clarity ("I'm specifying a resource type, not just a random string"), so I'm personally in favor of leaving it as the original author wrote it unless one advantage seems to strongly outweigh the other in a specific use.

@brendankenny brendankenny dismissed their stale review July 17, 2018 18:20

meant to be a comment, not an approval :)

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

I too have almost no preference here, so I'm good with whatever. With type checking it makes little difference to me. Bringing in the enum is another require but can make it a bit more explicit where the value is coming from.

@@ -46,7 +47,7 @@ class BootupTime extends Audit {
/** @type {Set<string>} */
const urls = new Set();
for (const record of records) {
if (record.resourceType && record.resourceType === 'Script') {
if (record.resourceType && record.resourceType === NetworkRequest.TYPES.Script) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

we can simplify this more now too, just record.resourceType === :)

@@ -87,7 +87,7 @@ class OptimizedImages extends Gatherer {

seenUrls.add(record.url);
const isOptimizableImage = record.resourceType &&
record.resourceType === 'Image' &&
record.resourceType === NetworkRequest.TYPES.Image &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

same here :)

@wardpeet
Copy link
Collaborator Author

i'll just leave it be and just fix the ones I already have in this PR.

I thank the all seeing eye of @patrickhulce 😄

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

WFM thanks for bringing it up! :)

@patrickhulce patrickhulce merged commit fd6de76 into master Jul 17, 2018
@patrickhulce patrickhulce deleted the bug/fix-resource-typings branch July 17, 2018 22:04
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.

None yet

3 participants