clients(viewer): add support for flow reports #13260
Conversation
| @@ -43,7 +43,7 @@ <h1 class="viewer-placeholder__heading">Lighthouse Report Viewer</h1> | |||
|
|
|||
| <input id="hidden-file-input" type="file" hidden accept="application/json"> | |||
|
|
|||
| <script src="src/bundled.js"></script> | |||
There was a problem hiding this comment.
Just spent the better part of an afternoon/evening on this. bundled.js is created as an ESM but we weren't loading it with type="module":
lighthouse/build/gh-pages-app.js
Line 159 in abb88b3
It wasn't a problem before because
bundle.js didn't use any static import/export statements. That changed when we added the flow renderer to the bundle.
The flow renderer imports shated/localization/format.js, which is also used in the dynamically imported i18n-module.js. So now format.js is used in the initially loaded code and the dynamically loaded i18n bundle.
This creates a new main-xxxx.js bundle that is imported in bundle.js and the i18n bundle. At this point the browser gets mad because because bundle.js is using a static import even though the script tag didn't have type="module".
Is there a reason type="module" was not added originally? If so, we might get around this by loading the flow assets dynamically.
There was a problem hiding this comment.
background: #10148 (comment)
tldr: adding type=module should be fine
my main concern previously was that concating multiple esm bundles would not work, and so I thought best to not mark the script element as a module so we did not accidentally rely on that. however on second thought I do think concat'ing multiple es module bundles (which we don't do now...) would be fine because imports are simply hoisted. 2 months ago I hadn't realized that, thinking they would error.
nice to know that rollup with extract these out into a separate file and load it. maybe we should add a link preload element for it though? concerned about the delayed loading here. Is there a way to enumerate all the static imports from the main bundle and emit link elements for it inside build-gh-pages-app.js?
There was a problem hiding this comment.
Is there a way to enumerate all the static imports from the main bundle and emit link elements for it inside build-gh-pages-app.js?
I think so looks like each output chunk has a convenient isDynamicEntry flag.
| @@ -16,7 +16,7 @@ | |||
| <link rel="stylesheet" href="styles/bundled.css"> | |||
| <link rel="canonical" href="https://googlechrome.github.io/lighthouse/viewer/"> | |||
| </head> | |||
| <body class="lh-vars"> | |||
There was a problem hiding this comment.
Do we need this? lh-vars is added by to the root element by the report renderer.
| _renderFlowResult(json, root) { | ||
| renderFlowReport(json, root); | ||
| // Install as global for easier debugging. | ||
| window.__LIGHTHOUSE_FLOW_JSON__ = json; |
There was a problem hiding this comment.
This same global is used in the standalone report. Should we just reuse __LIGHTHOUSE_JSON__ in the viewer? Otherwise the console could be printing a mix of __LIGHTHOUSE_JSON__ and __LIGHTHOUSE_FLOW_JSON__.
There was a problem hiding this comment.
Keeping them separated is my preference
adamraine
changed the title
Co-authored-by: Connor Clark <cjamcl@google.com>
|
Excellent work! 🎉 🌮 |
This is not a huge priority for v9 or CDS, but we should land soon so people can easily share flow reports via gist and such.
Quick gist example:
https://lighthouse-git-flow-viewer-googlechrome.vercel.app/gh-pages/viewer/?gist=a2b3f815f1a0f4a0cf69e5b6f50326be
#11313