Remove BOM encoding from manifest #2175
Conversation
wardpeet
force-pushed
the
BOM-manifest
branch
2 times, most recently
from
022422b
to
1c213ab
Compare
|
I didn't read the full history of #1702 but i have some recent news: AFAIK the chrome manifest parser had no problem with BOM but devtools' application panel did. (is that confirmed?) so afaik, there won't be a problem with BOM parsing in chrome anymore. With that status, should we still throw in this situation? |
|
Let me give it a spin if we get our manifest back from devtools. Before the devtools fix it gave a null manifest I'll close it myself when i've tested it |
|
hmmmm! we might still have a problem then. the devtools fix was only in the frontend, so yeah looks like we'll need this patch afterall. (real chrome doesn't use the protocol method. that's the difference here) |
| const isBomEncoded = Buffer.from(response.data).slice(1, 4) | ||
| .equals(new Buffer(BOM_ENCODING)); | ||
| if (isBomEncoded) { | ||
| throw new Error('Manifest is encoded with BOM. Please remove the BOM encoding'); |
There was a problem hiding this comment.
rather than throw error let's just remove the BOM and let it proceed.
wardpeet
changed the title
|
@paulirish fixed! ;) issue was still present in lighthouse |
|
Nice I also tested with the fixture in https://bugs.chromium.org/p/chromium/issues/detail?id=706926 which is what we validated the devtools fix against.
|
| }; | ||
|
|
||
| const promises = []; | ||
| promises.push(manifestGather.afterPass(getDriver(manifestWithBOM)) |
There was a problem hiding this comment.
I don't think this will throw even if still bad...won't it show with an "isn't valid JSON" debugString?
It would be good to check the result from afterPass...result.raw should have the raw string passed in, so it should have length equal to manifestWithBOM minus the BOM. And it would be good to check some of the parsed values to make sure it's being parsed
There was a problem hiding this comment.
damn forgot to change the test text as we throwed before it should say pass when manifest has BOM
| @@ -36,6 +37,12 @@ class Manifest extends Gatherer { | |||
| afterPass(options) { | |||
| return options.driver.getAppManifest() | |||
| .then(response => { | |||
| const isBomEncoded = Buffer.from(response.data).slice(0, BOM_ENCODING.length) | |||
| .equals(new Buffer(BOM_ENCODING)); | |||
There was a problem hiding this comment.
what's the compatibility like if we skip the Buffer slicing for the check (so files without a BOM don't have to go through that) and do const isBomEncoded = response.data.charCodeAt(0) === 65279;. Does that work cross platform?
There was a problem hiding this comment.
I can test on windows, linux & mac just to be sure
|
@brendankenny tested your change on Windows 10, mac and linux and seems to work for me. |
|
Awesome. Thanks for testing that out. We'll let this PR and TTFB chill until we cut 2.0, and then we'll incorporate. Sorry for making you wait! |
|
@paulirish you never have to say sorry 😉 It's no urgent bug so it can wait |
|
@brendankenny everything should be ready to go |
There was a problem hiding this comment.
looks good!
I pushed a last commit for some minor promise formatting in the test file just to expedite landing :)
Otherwise, verified it works with a real run with BOM-encoded manifest and that the new test fails if the BOM is removed from the test manifest.
I'll wait for Travis, but otherwise LGTM!
Codecov Report
@@ Coverage Diff @@
## master #2175 +/- ##
=========================================
Coverage ? 83.69%
=========================================
Files ? 177
Lines ? 5785
Branches ? 802
=========================================
Hits ? 4842
Misses ? 923
Partials ? 20
Continue to review full report at Codecov.
|
Fixes issue #1702