clients: simplify viewer build process #6426
Conversation
brendankenny
requested review from
ebidel,
patrickhulce and
paulirish
as code owners
| @@ -4,7 +4,7 @@ npm-debug.log | |||
| .vscode | |||
| .tmp | |||
|
|
|||
| dist | |||
There was a problem hiding this comment.
is there significance to this?
There was a problem hiding this comment.
I had lighthouse-viewer/dist left after this PR from an earlier build and it'll just stay there since it's gitignored. This will kind of alert that it's unnecessary now, but otherwise not much significance :)
| } | ||
| } | ||
|
|
||
| run(); |
There was a problem hiding this comment.
I was about to write something along the lines of shame we have to have all this code, but then I saw that it's even fewer lines than the gulpfile 😆
| @@ -70,6 +66,9 @@ | |||
| "build-proto-roundtrip": "cd proto/scripts && PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=cpp PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION_VERSION=2 python json_roundtrip_via_proto.py" | |||
| }, | |||
| "devDependencies": { | |||
| "@firebase/app-types": "0.3.1", | |||
There was a problem hiding this comment.
😬
yeah, this is only for the types. We use so little of it, maybe we should make our own types version locally
| @@ -224,7 +224,6 @@ git clone https://github.com/GoogleChrome/lighthouse | |||
|
|
|||
| cd lighthouse | |||
| yarn | |||
| yarn install-all | |||
| const fs = require('fs'); | ||
| const path = require('path'); | ||
| const {promisify} = require('util'); | ||
| const readFileAsync = promisify(fs.readFile); |
There was a problem hiding this comment.
just curious, is there a particular reason we're using the async versions in scripts?
There was a problem hiding this comment.
just curious, is there a particular reason we're using the async versions in scripts?
just a vague sense that we don't have to block on reading/writing, but just tried it (still on an SSD) with all sync and the files are so small/the build is so quick, it makes zero difference :) With await now they're roughly interchangeable, so it doesn't matter to me. Want to switch?
There was a problem hiding this comment.
Nah it's fine, I just tend to use the sync ones for scripts to not mess with the promisification and async in scripts when everything is pretty much serial anyway and was curious if there was another benefit
brendankenny
force-pushed
the
buildviewer
branch
from
d343c67
to
50eb44a
Compare
|
oh it's so pretty, all in one place @paulirish |
Quick one-off to get viewer matching the clients after #6344 and #6295.
This eliminates
gulp(we similarly barely used it here), moves the build script tobuild/, writes the output of the viewer build process todist/viewer/, and there's no longer a need foryarn install-all.dist output is exactly the same as master except that for some reason in
viewer.jswe end up with a file-leveluse strictinstead of a scattering of function-leveluse stricts. Not sure why that happened (I assume there's an uglify setting), but it's strictly better, to that's fine :)I'd argue for shortening
lighthouse-viewer/toviewer/and possibly moving intoclients/while we're still pre-v4. Viewer is arguably a client but it also arguably dilutes that definition (currently a file that bundles all of lighthouse core), so we may or may not want to do that.Also, we could get rid of uglify from viewer and use the same simple babel "minification" we use on the other bundles. It's only adds 15KB over 233KB (or 5KB over 51KB after gzip) to lose uglify. OTOH, we could leave the same until someone comes along and redoes our bundling/minification step.