-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
[bundlephobia] Added a shield for calculating npm bundle sizes #1391
Conversation
Generated by 🚫 dangerJS |
I'm not exactly sure why the CI check failed. Looks like some sort of an error related to |
Yea, you're right, it's unrelated. I opened #1392 to increase the timeout on that test. It times out fairly often. |
Hey, are there any blockers for this to get merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this! Here are a few comments for you.
lib/all-badge-examples.js
Outdated
'node', | ||
'npm', | ||
'bundle', | ||
'size' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can just include 'node'
here and remove the others. Substrings of the title will be matched automatically.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
server.js
Outdated
// C: /bundlephobia/(min|gzip)/@:scope/:package.:format | ||
// D: /bundlephobia/(min|gzip)/@:scope/:package/:version.:format | ||
const capturedResultType = match[1]; | ||
const capturedScopeWithoutAtSign = match[2]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much as I love verbose names, I think you could drop captured
from these, and shorten this one to scope
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool.
server.js
Outdated
@@ -7715,6 +7715,53 @@ camp.route(/^\/nsp\/npm\/(?:@([^/]+)?\/)?([^/]+)?(?:\/([^/]+)?)?\.(svg|png|gif|j | |||
} | |||
})); | |||
|
|||
// bundle size for npm packages | |||
camp.route(/^\/bundlephobia\/(min|gzip)\/(?:@([^/]+)?\/)?([^/]+)?(?:\/([^/]+)?)?\.(svg|png|gif|jpg|json)?$/, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minified and gzipped aren't exclusive. Does gzip mean minified and gzipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then how about we make it min
and min-gzip
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another commonly used term for this is "minzip"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that works and seems clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
server.js
Outdated
}); | ||
} | ||
|
||
getBundlephobiaResults(capturedScopeWithoutAtSign, capturedPackageName, capturedVersion); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What advantage does making this basically an IIFE provide?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inlined the function
server.js
Outdated
badgeData.colorscheme = 'red'; | ||
} else { | ||
badgeData.text[1] = prettyBytes(showMin ? body.size : body.gzip); | ||
badgeData.colorscheme = 'blue'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you put this in a try block to handle the case where the JSON is malformed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a check for typeof body !== 'object' || body === null
service-tests/bundlephobia.js
Outdated
.get(noExistPackage) | ||
.expectJSON({ | ||
name: 'minified size', | ||
value: 'PackageNotFoundError', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minified size | package not found or even bundlephobia | package not found would be more stylistically consistent with our error badges.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These error codes are returned by the bundlephobia service, and there are multiple such types - EntryPointError
| PackageNotFoundError
| VersionMismatchError
etc. I'd prefer to keep them the same to enable easier debugging when issues are filed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's sufficient to re-run the failing request when an issue is filed. One of the goals of Shields is to provide consistent and concise badges, which applies to error messages too.
service-tests/bundlephobia.js
Outdated
const withoutErrorSizesGzip = { | ||
A: Joi.string().regex(/^\d+[.]?\d+ kB$/), | ||
B: '3.58 kB', | ||
C: Joi.string().regex(/^\d+[.]?\d+ kB$/), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use isFileSize
for this.
const withoutErrorSizeMin = withoutErrorSizesMin[format] | ||
|
||
const withoutErrorGzip = withoutErrorsGzip[format] | ||
const withoutErrorSizeGzip = withoutErrorSizesGzip[format] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is nice and DRY, though it's a bit difficult to follow what you're doing. Also not sure it's necessary to be exhaustive in testing these combinations.
Could I suggest a couple ways of making it easier to understand? One would be to reduce this to 5 success cases (four min and one gzip) and 1–3 failure cases, and lay out each test explicitly. The other would be to basically invert your data objects, making an array of objects with format
, withoutErrorMin
, etc. as keys.
Are you willing to make the change to standardize the errors? If not I could pick this up from here. |
Since I might have new error types in the future, I've added a regex based formatter that basically does this conversion => The CI seems to be tripping on something unrelated. |
Okay, I'll pick that up. I noticed the services-pr failure, which I think I fixed in #1454. If you merge master I think that should clear. The failure in main should be fixable with |
9ace8bd
to
2befb65
Compare
8077a32
to
319b812
Compare
Lint errors / tests are passing now. |
@pastelsky @paulmelnikow - I want to help you with this PR, can I do that somehow? |
@tlaziuk That would be great! Would you like to take a shot at refactoring the tests? If so you could just grab these commits, merge in master, address the comments, and open a new PR. |
Let's finish this up in #1481. |
A shield for bundle sizes of front end npm packages, as discussed over here - #941