-
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
extension: split browserifying and extension bundling into separate scripts #6295
Conversation
await cpy('app/manifest.json', distDir); | ||
|
||
// locales (currently non-functional) | ||
await cpy('_locales/**', `../${distDir}`, { |
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 we add our locales to the extension, it might still make sense to use these files to do things like localize the webstore descriptions and extension name and stuff, so I left them in
const fs = require('fs'); | ||
const path = require('path'); | ||
|
||
const LighthouseRunner = require('../lighthouse-core/runner'); |
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 all almost exactly the same as it was in the gulpfile, just unindented a few times. One good piece of news: our astw
hack is no longer necessary because insert-module-globals
moved off off lexical-scope
in browserify/insert-module-globals#76 :)
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, and no objection to future moving to a different bundler or whatever. Left exactly the same to stay out of that question for now :)
It should be even easier since bundling is isolated from the specifics of extension building.
}, | ||
"devDependencies": {}, |
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.
they were all devDeps (so no npm package penalty), we no longer support an environment requiring only one version of any npm module, there were even fewer deps now, and I got a few typescript conflicts between here and the parent directory, so it made sense to move them all up to the root directory
(vast majority of the line count is |
* Minimally minify a javascript file, in place. | ||
* @param {string} filePath | ||
*/ | ||
function minifyScript(filePath) { |
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.
should we call this minify? 😛 we minify but not really.. 🙄
should we call it |
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.
🛎 🔔the gulp is 💀 ! 🎉
"bundlesize": "^0.14.4", | ||
"chalk": "^2.4.1", | ||
"codecov": "^2.2.0", | ||
"commitizen": "^2.9.6", | ||
"conventional-changelog-cli": "^1.3.4", | ||
"coveralls": "^2.11.9", | ||
"cpy": "^7.0.1", |
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.
not a big deal, but I generally preferred to use https://github.com/shelljs/shelljs instead of bringing in all the individual commands (cpy,del,make-dir,etc) with different APIs to remember
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 we should consider that for the future. del
and make-dir
bring in a crazy number of deps :) Some of the simpler bash scripts could switch over as well so they work cross platform.
I get this question now :) I can rename it if you feel strongly, but we'll probably just switch it back when #6299 is fixed and the file only builds the extension. It seems somewhat ok for now because the status quo is that "building the extension" (e.g. |
I don't have strong feelings so feel free to keep it for now |
This sets the stage for splitting up the extension from the
devtools-entry.js
andlightrider-entry.js
files. The only thing they have in common is the browserifying of an entry-point script, so it makes sense to pull that out into its own file.Also removes all the gulp stuff, because all our gulp tasks were either copying files from one location to another (gulp not needed), or tapping into the stream and modifying the files individually (negating the whole point of gulp :). With the
build-extension.js
script now 95% just putting together the extension assets, this is easy to do.The output of the various bundles and their location doesn't change with this PR. I'll file an issue with ideas for reorganizing so we can all bikeshed :)