-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
docs(releasing): minor updates #5345
Conversation
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 apparently missed when we started npmignoring smokehouse. We need to be sure we aren't shipping something buggy, so it seems like this needs to be one of
- don't npmignore smokehouse. It's not that big, and if it somehow is we could do something like just include a subset with that don't have large local files
- write a test to automate testing a gzipped lighthouse package
- include a checklist here of expected audit results for paulirish.com and example.com so we verify that it's not only running but that it's running correctly
wait, it's still not ignored? Line 23 in 520f9ac
|
f2c8c08
to
da4ef7c
Compare
@brendankenny blast from the past. ;) changes
|
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.
🌬️LGTM!
lighthouse-cli/types/*.js | ||
lighthouse-cli/types/*.map | ||
# keep smokehouse tests, etc | ||
!lighthouse-cli/test |
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.
how much bigger does our package end up?
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.
oh this was not fun. turns out yarn pack
builds things VERY differently than npm pack
.
and npm pack
gives basically the same package before and after this PR. wonderful.
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.
turns out it's smaller now. because the npmignore in master includes all of lighthouse-cli already.
so aint that somethin
update: see #5345 (comment)