-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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(gulp): rename output files #6179
Conversation
package.json
Outdated
@@ -157,7 +157,15 @@ | |||
}, | |||
"bundlesize": [ | |||
{ | |||
"path": "./lighthouse-extension/dist/scripts/lighthouse-background.js", | |||
"path": "./lighthouse-extension/dist/scripts/lighthouse-dt-bundle.js", |
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.
unsure which one to check or if we should check them all 3
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.
let's check ext bundle since it's the most of our code but without worrying about string changes
I tried using gulp-rename but it did not copy my whole file for some reason so I switched to a tap instance. |
aeb51d6
to
174e085
Compare
@paulirish could you check roll out to devtools? |
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.
works great. thx!
package.json
Outdated
@@ -157,7 +157,15 @@ | |||
}, | |||
"bundlesize": [ | |||
{ | |||
"path": "./lighthouse-extension/dist/scripts/lighthouse-background.js", | |||
"path": "./lighthouse-extension/dist/scripts/lighthouse-dt-bundle.js", |
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.
let's check ext bundle since it's the most of our code but without worrying about string 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.
excited about this!
@@ -93,8 +93,8 @@ git push --tags | |||
echo "Rebuild extension and viewer to get the latest, tagged master commit" | |||
yarn build-all; | |||
|
|||
# zip the extension files, but remove lh-background as it's not needed | |||
cd lighthouse-extension; command rm -f dist/scripts/lighthouse-background.js; gulp package; cd .. |
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.
we should really update gulp package
to do this for us. I've definitely never remembered to delete the other files :):)
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'll add a new PR that fixes this.
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.
@brendankenny see #6210
lighthouse-extension/gulpfile.js
Outdated
const isDevtools = file => file.endsWith(CONSUMERS.DEVTOOLS); | ||
const isExtension = file => file.endsWith(CONSUMERS.EXTENSION); | ||
const isDevtools = file => | ||
file.endsWith(CONSUMERS.DEVTOOLS.src) || file.endsWith(CONSUMERS.DEVTOOLS.dist); |
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.
can the dist
file really ever go through this function?
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.
probably not but wanted to be save but i'm removing it
@@ -6,12 +6,12 @@ | |||
'use strict'; |
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 you missed a /
in the git mv
:)
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 nice thanks
@@ -6,7 +6,7 @@ | |||
'use strict'; | |||
|
|||
const lighthouse = require('../../../lighthouse-core/index'); | |||
const background = require('./lighthouse-background'); | |||
const background = require('./devtools-entry'); |
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.
it's unfortunate to have to keep calling this background
, but I can't think of a better name right now :)
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.
if I move getDefaultConfigForCategories
& getDefaultCategories
to it's own file called one of them below I could rename the variable which gives more meaning to it.
config.js
,helpers/config.js
optionsHelper.js
runOptions.js
lighthouseOptions.js
- ...
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.
yeah, I think that makes sense, but I think we should just wait on it (at least until LR shakes out and we can fully see the shape of these files for the medium-term future).
Summary
renaming consumer's files:
devtools-entry.js => lighthouse-dt-bundle.js
extension-entry.js => lighthouse-ext-bundle.js
lightrider-entry.js => lighthouse-lr-bundle.js
also removed debug from browserify which I accidentally added.
sizes are intact, there is a small diff but it's just whitespace
Related Issues/PRs
#6170