don't emit script tags in devtools report. #1105
Conversation
| </head> | ||
| <body> | ||
|
|
||
| <script> | ||
| // expose the original results | ||
| <!-- |
There was a problem hiding this comment.
shall we just remove? The JSON embed makes the html report reallllly big.
There was a problem hiding this comment.
i feel like it's useful to keep around, but I don't have the strongest usecase just yet.
e.g. what if a report-viewer customer wants to d/l the JSON from a report they are looking at, so they can archive it and compare past results?
but.. I'm OK with nuking it for now and only bringing it back when there's definite need.
There was a problem hiding this comment.
You're right. We'll need the JSON embedded for that use case. We can add it back later :)
| @@ -173,7 +173,8 @@ class ReportGenerator { | |||
| * @return {string} | |||
| */ | |||
| getReportJS() { | |||
| return fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'), 'utf8'); | |||
| const scriptSrc = fs.readFileSync(path.join(__dirname, './scripts/lighthouse-report.js'), 'utf8'); | |||
| return `<script>${scriptSrc}</script>`; | |||
There was a problem hiding this comment.
can we keep getReportJS() the same and use handlebars to conditionally write them if there's a script?
| @@ -261,14 +262,16 @@ class ReportGenerator { | |||
| }); | |||
|
|
|||
| const template = Handlebars.compile(this.getReportTemplate()); | |||
| reportContext = reportContext || 'extension'; // could be: devtools, extension | |||
There was a problem hiding this comment.
why doesn't it have reportContext for the extension at this point?
There was a problem hiding this comment.
it should, but I can imagine someone using generateHTML on their own, and not setting the second argument.
would you rather this go forward as undefined or we throw?
There was a problem hiding this comment.
ah, that's fine (and I just noticed you were just moving it up from where it already was). Maybe just add a note about the default to the jsdoc?
There was a problem hiding this comment.
it appears this is just used for styling, and just for devtools embed right now. I wonder if we should rename it to something to indicate that. reportStyle or whatever
There was a problem hiding this comment.
(maybe reportContext means that already, but without context I was thinking it was more "what generated this report" rather than "what's managing the viewer for this report")
There was a problem hiding this comment.
k. added more detail to jsDoc and moved the default value to top of function. this work for you?
I could also rename to reportStyle if you prefer, works for me.
There was a problem hiding this comment.
I could also rename to reportStyle if you prefer, works for me.
as I read it today reportContext makes perfect sense, so who knows :)
|
ptal |
|
you should rebase. Eric removed those |
paulirish
force-pushed
the
scripttagsno
branch
from
f7ce828
to
49877e9
Compare
paulirish
force-pushed
the
scripttagsno
branch
from
49877e9
to
0b08a2e
Compare
|
rebased and all that jazz @brendankenny |
* don't emit script tags in devtools report.
Our devtools integration is that the report is hosted in and iframe (with a blob url).
DevTools has a CSP policy: https://github.com/ChromeDevTools/devtools-frontend/blob/master/front_end/inspector.html#L10
It prevents inline scripts from running.
That ends up throwing an error for our report. While there may be some ways to handle this at a CSP layer, I'd prefer, in the short term, to make our report function without javascript.
The report only needs JS for the
window.print()which is hidden from the devtools version (viareportContext).This PR tweaks things so we don't ship a script tag to devtools; and we only ship 1 script tag to the extension.