report: gzip treemap data#12519
Conversation
| fs.readFileSync(__dirname + '/renderer/report-renderer.js', 'utf8'), | ||
| fs.readFileSync(__dirname + '/renderer/i18n.js', 'utf8'), | ||
| fs.readFileSync(__dirname + '/renderer/base64.js', 'utf8'), | ||
| fs.readFileSync(require.resolve('pako/dist/pako_deflate.min.js'), 'utf-8'), |
There was a problem hiding this comment.
this is probably problematic for devtools...
There was a problem hiding this comment.
seems like we could skip deflate in most of our environments and be ok falling back to non-gzipped if compressionstreams aren't supported? now that I say this I wonder how much value all of this gzip business has to begin with...
it's the inflate side where they're sharing it out that it's critical to support if we support deflate anywhere
There was a problem hiding this comment.
seems like we could skip deflate in most of our environments and be ok falling back to non-gzipped if compressionstreams aren't supported?
done.
now that I say this I wonder how much value all of this gzip business has to begin with...
I got 10kb for the treemap options, vs ~60kb if no gzip. Either way, it's a really long URL. And both are too long for every URL shortener service I've tried.
I'm fine without the gzip. @paulirish ?
|
|
||
| /* global self btoa atob window CompressionStream Response */ | ||
|
|
||
| const btoaIso = typeof btoa !== 'undefined' ? |
There was a problem hiding this comment.
prefer btoa_?
There was a problem hiding this comment.
ah gotcha, yeah btoa_ was what I had in my head, or btoaIsomorphic :D
There was a problem hiding this comment.
i think there's fair consensus that 'atob' is one of the worst named APIs of the web… so i don't love calling our stuff that.
should either go full out asciiToBinary if you wanna honor that API or otherwise something like textToBase64String. I'm fine leaving off the isomophoric suffix, but could add a comment above to explain why we do it so funny
There was a problem hiding this comment.
i think there's fair consensus that 'atob' is one of the worst named APIs of the web… so i don't love calling our stuff that.
I totally agree, and I fully support never naming our APIs atob or btoa.
However, internal to this I think there's value in reflecting what underlying methods are being used to perform this so I'd strongly prefer asciiToBinary to a complete rename. We already did the complete useful rename for the API surface of this module :)
| /** The title of the stack pack. */ | ||
| title: string; | ||
| /** A base64 data url to be used as the stack pack's icon. */ | ||
| /** A TextEncoding data url to be used as the stack pack's icon. */ |
| * @license Copyright 2021 The Lighthouse Authors. All Rights Reserved. | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. | ||
| */ |
There was a problem hiding this comment.
speaking of #12570 (comment), there used to be a thing with the closure license checker and the files in renderer/, which is why they all have the usual long(er) form of the Apache 2 header. Have you tried importing this yet? Or is that no longer flagged
edit: or I guess it was license-header line length? Weird. But still, same question :)
There was a problem hiding this comment.
Seems like something that would only show up in the CL check, which I never did (just built and manually tested). If that's the case, I'll edit in the import and just make a new PR here that can land whenever.
There was a problem hiding this comment.
Seems like something that would only show up in the CL check, which I never did (just built and manually tested). If that's the case, I'll edit in the import and just make a new PR here that can land whenever.
Is there any downside to changing it now?
There was a problem hiding this comment.
snippet renderer has the same comment and is in BUILD, so it should be fine
demo