Include commit hash and version to browser extension#2236
Include commit hash and version to browser extension#2236brendankenny merged 10 commits intoGoogleChrome:masterfrom
Conversation
wardpeet
force-pushed
the
feat/banner-hash
branch
from
b614568 to
a2a83d5
Compare
|
|
wardpeet
force-pushed
the
feat/banner-hash
branch
from
4df94cf to
3357c18
Compare
|
Rebased and ready to go! |
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
Review! This is kind of ridiculously long in coming, but we need this :)
Mostly comments on how we could maybe simplify this. Let me know what you think.
| return chrome.runtime.getManifest().version; | ||
| } | ||
|
|
||
| function getLighthouseRevision() { |
There was a problem hiding this comment.
bikeshed: can we call this getLighthouseCommitHash or something like that (and update references to REVISION below)? I wasn't sure what "revision" was referring to until I got to where it was defined :)
**Lighthouse Revision** below could be **Lighthouse Commit**, for instance.
Just an idea, though. Open to other ways people refer to the exact commit
There was a problem hiding this comment.
I don't mind what it's called
lighthouse-extension/gulpfile.js
Outdated
| bundle.transform('browserify-versionify', { | ||
| placeholder: '__REVISION__', | ||
| version: REVISION, | ||
| }); |
There was a problem hiding this comment.
could we drop the insertGlobalVars/browserify-versionify parts and just do a .pipe(gulpReplace('__REVISION__', REVISION))?
There was a problem hiding this comment.
.pipe(gulpReplace('__REVISION__', REVISION))
we could make the string to replace even more unique if necessary
lighthouse-extension/gulpfile.js
Outdated
| file.contents = new Buffer(minified); | ||
| return file; | ||
| })) | ||
| .pipe(banner(BANNER)) |
There was a problem hiding this comment.
Since it's a build step (and so just devDependencies), I personally don't particularly mind using yet another gulp library instead of writing our own function. It seems like gulp-header would let you inject this comment pretty easily at the top.
There was a problem hiding this comment.
Sure, I can take a dependency, just didn't want to introduce another one but yeah dev dependency doesn't really matter
package.json
Outdated
| "devDependencies": { | ||
| "@types/node": "^6.0.45", | ||
| "babel-core": "^6.16.0", | ||
| "browserify-versionify": "^1.0.6", |
There was a problem hiding this comment.
I think we want this (or whatever new deps we add) in lighthouse-extension/package.json so that it's scoped to only install for those who are working on/with the extension
wardpeet
force-pushed
the
feat/banner-hash
branch
from
3357c18 to
c0c135c
Compare
|
@brendankenny @paulirish just notifying you guys. Pretty sure it's ok now. |
brendankenny
left a comment
brendankenny
left a comment
There was a problem hiding this comment.
two small last changes to this but I think it's good otherwise
yarn.lock
Outdated
| version "0.1.1" | ||
| resolved "https://registry.yarnpkg.com/find-index/-/find-index-0.1.1.tgz#675d358b2ca3892d795a1ab47232f8b6e2e0dde4" | ||
|
|
||
| find-root@^0.1.1: |
There was a problem hiding this comment.
is this still necessary? Seems like changes to this file should have been dropped since no changes to root package.json
There was a problem hiding this comment.
I cleaned the yarn.lock with executing yarn, should be good to go
lighthouse-extension/gulpfile.js
Outdated
| const distDir = 'dist'; | ||
|
|
||
| const VERSION = pkg.version; | ||
| const COMMITHASH = require('child_process') |
There was a problem hiding this comment.
nit: maybe COMMIT_HASH?
wardpeet
force-pushed
the
feat/banner-hash
branch
from
5f3368b to
171e837
Compare
|
reverted part of a comment removed by a merge |
2 participants
fixes #2190