report: move renderer code to report/ by connorjclark · Pull Request #12690 · GoogleChrome/lighthouse

connorjclark · 2021-06-24T06:36:23Z

Moving the renderer code from lighthouse-core/report to report/, and simplifying the directory structure. Better separation (core vs report), and provides room for us to continue to grow the report renderer (upcoming refactor + FR changes + maybe LHCI diff view?).

Renamed report-template.html to standalone-template.html, as I always confused it with templates.html. Also matches the standalone report terminology in the upcoming report renderer refactor.

~~Need to follow with a strings import.~~ This PR includes a strings import.

report
├── README.md
├── assets
│   ├── standalone-template.html
│   ├── styles.css
│   └── templates.html
├── renderer
│   ├── category-renderer.js
│   ├── crc-details-renderer.js
│   ├── details-renderer.js
│   ├── dom.js
│   ├── element-screenshot-renderer.js
│   ├── i18n.js
│   ├── logger.js
│   ├── performance-category-renderer.js
│   ├── psi.js
│   ├── pwa-category-renderer.js
│   ├── report-renderer.js
│   ├── report-ui-features.js
│   ├── snippet-renderer.js
│   ├── text-encoding.js
│   └── util.js
├── report-assets.js
├── report-generator.js
└── test
    ├── renderer
    │   ├── category-renderer-test.js
    │   ├── crc-details-renderer-test.js
    │   ├── details-renderer-test.js
    │   ├── dom-test.js
    │   ├── element-screenshot-renderer-test.js
    │   ├── i18n-test.js
    │   ├── performance-category-renderer-test.js
    │   ├── psi-test.js
    │   ├── pwa-category-renderer-test.js
    │   ├── report-renderer-test.js
    │   ├── report-ui-features-test.js
    │   ├── snippet-renderer-test.js
    │   ├── text-encoding-test.js
    │   └── util-test.js
    └── report-generator-test.js

4 directories, 36 files

connorjclark · 2021-06-24T06:37:15Z

-
-const TEMPLATE_FILE = fs.readFileSync(__dirname +
-    '/../../../../report/html/templates.html', 'utf8');
+const reportAssets = require('../../report-assets.js');


minor tweak I did in these test files was dropping fs for report-assets.js

connorjclark · 2021-06-24T06:38:59Z

  if (collisions > 0) {
    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
+    assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);


idk why this changed

that's somewhat concerning... :/

can we do a diff?

this is what I see on master

"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text', 'Document has a valid $MARKDOWN_SNIPPET_0$', 'Document has a valid $MARKDOWN_SNIPPET_0$', 'Potential Savings', 'Potential Savings', 'Failing Elements', 'Failing Elements', 'URL', 'URL', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.', 'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.', 'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text', 'Document has a valid $MARKDOWN_SNIPPET_0$', 'Potential Savings', 'Failing Elements', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.', 'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

diff https://www.diffchecker.com/TFvWBE9j

hm, that's 10 more missing than there should be, I had to add collisionStrings.push where the first collisions++ is

collectAllStringsInDir does have some local data per directory, and renderer strings moved to a new folder, so that must be why. no problem here i guess ... except maybe in the "collision detection" code.

I didn't follow up after https://github.com/GoogleChrome/lighthouse/pull/12441/files#r630521367 :(

connorjclark · 2021-06-24T06:39:36Z

@@ -1,30 +1,26 @@
 # Lighthouse HTML Report Renderer


mostly just updated links here.

patrickhulce

thanks for splitting this up, I like the new directory structure 👍

patrickhulce · 2021-06-24T16:06:38Z

  if (collisions > 0) {
    console.log(`MEANING COLLISION: ${collisions} string(s) have the same content.`);
-    assert.equal(collisions, 30, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);
+    assert.equal(collisions, 28, `The number of duplicate strings have changed, update this assertion if that is expected, or reword strings. Collisions: ${collisionStrings.join('\n')}`);


that's somewhat concerning... :/

can we do a diff?

this is what I see on master

"When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", 'ARIA $MARKDOWN_SNIPPET_0$ elements have accessible names', 'ARIA $MARKDOWN_SNIPPET_0$ elements do not have accessible names.', "When an element doesn't have an accessible name, screen readers announce it with a generic name, making it unusable for users who rely on screen readers. $LINK_START_0$Learn more$LINK_END_0$.", '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text', '$MARKDOWN_SNIPPET_0$ elements do not have $MARKDOWN_SNIPPET_1$ text', 'Document has a valid $MARKDOWN_SNIPPET_0$', 'Document has a valid $MARKDOWN_SNIPPET_0$', 'Potential Savings', 'Potential Savings', 'Failing Elements', 'Failing Elements', 'URL', 'URL', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.', 'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.', 'Consider using a $LINK_START_0$plugin$LINK_END_0$ or service that will automatically convert your uploaded images to the optimal formats.', 'Consider uploading your GIF to a service which will make it available to embed as an HTML5 video.'

connorjclark · 2021-06-24T18:40:13Z

can we do a strings import w/o actually committing? I think so. there's a test that is failing b/c of missing renderer strings in en-XA

brendankenny

are we seriously going to do another lighthouse-*/ directory? :P

report/!

connorjclark · 2021-06-24T21:04:39Z

Haha I agree, but was gonna suggest renaming all of those in another PR and did lighthouse-report just to match. Would lighthouse-core to core be breaking?

Could also just make it report now. Wdyt

brendankenny · 2021-06-24T21:15:54Z

ha, I wasn't sure if anyone else would agree :) It probably makes sense to start with just report/ and do it here, but not sure if anyone else cares

connorjclark · 2021-06-25T00:13:37Z

ok i'm doing it

connorjclark · 2021-06-25T00:31:41Z

imported strings too

connorjclark · 2021-06-25T02:08:06Z

@paulirish can you turn off CDT tests for a bit? at least until the renderer changes land sometime next week. this PR renames template.html to standalone-template.html, which means some BUILD crap in CDT needs to change too, but we can't do it all at once. and then, many things need to change in BUILD for es modules to land. so best to disable for a bit.

connorjclark · 2021-06-25T02:16:12Z

@patrickhulce so we agree the string collision is not a real problem, and we will hash it out eventually (but no reason to block this PR). anything else?

connorjclark added 8 commits June 23, 2021 17:43

rename commit

2eae14e

rename

08556ec

update some paths

0cb7b49

rename paths in tests

89cc439

fix most tests

68b3003

strings

812bd70

minor doc tweaks

c1c27de

tweak

0ab79a7

connorjclark requested a review from a team as a code owner June 24, 2021 06:36

connorjclark requested review from patrickhulce and removed request for a team June 24, 2021 06:36

google-cla Bot added the cla: yes label Jun 24, 2021

connorjclark commented Jun 24, 2021

View reviewed changes

Comment thread tsconfig.json Outdated

connorjclark commented Jun 24, 2021

View reviewed changes

Comment thread lighthouse-report/test/proto-test.js Outdated

connorjclark commented Jun 24, 2021

View reviewed changes

devtools-bot assigned patrickhulce Jun 24, 2021

devtools-bot added the waiting4reviewer label Jun 24, 2021

connorjclark commented Jun 24, 2021

View reviewed changes

connorjclark mentioned this pull request Jun 24, 2021

☂️ Convert to ES modules #12689

Closed

17 tasks

tweak

d5fa816

vercel Bot deployed to Preview June 24, 2021 06:46 View deployment

forgot some paths

feb594d

vercel Bot deployed to Preview June 24, 2021 06:53 View deployment

lol

698e8ff

vercel Bot deployed to Preview June 24, 2021 06:58 View deployment

patrickhulce reviewed Jun 24, 2021

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Jun 24, 2021

movetest

20ac77e

vercel Bot deployed to Preview June 24, 2021 18:24 View deployment

brendankenny reviewed Jun 24, 2021

View reviewed changes

connorjclark mentioned this pull request Jun 24, 2021

misc(scripts): improve collision check in collect-strings #12697

Merged

connorjclark added waiting4reviewer and removed waiting4committer labels Jun 24, 2021

report/

3c0b416

vercel Bot deployed to Preview June 25, 2021 00:20 View deployment

connorjclark changed the title ~~report: move renderer code to lighthouse-report~~ report: move renderer code to report/ Jun 25, 2021

import strings

ee2774a

vercel Bot deployed to Preview June 25, 2021 00:31 View deployment

patrickhulce approved these changes Jun 25, 2021

View reviewed changes

connorjclark merged commit dddeb0d into master Jun 25, 2021

connorjclark deleted the move-report-code branch June 25, 2021 02:37

qmonmert mentioned this pull request Jul 14, 2021

Fix upgrade Lighthouse 8.0.0 -> 8.1.0 jhipster/generator-jhipster#15654

Merged

6 tasks

connorjclark mentioned this pull request Jul 22, 2021

tests: add report/ to tsconfig #12822

Merged

Conversation

connorjclark commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

connorjclark commented Jun 24, 2021

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

connorjclark commented Jun 24, 2021

Uh oh!

brendankenny commented Jun 24, 2021

Uh oh!

connorjclark commented Jun 25, 2021

Uh oh!

connorjclark commented Jun 25, 2021

Uh oh!

connorjclark commented Jun 25, 2021

Uh oh!

connorjclark commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

connorjclark commented Jun 24, 2021 •

edited

Loading