Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

report(flow): embedded lighthouse report #12989

Merged
merged 13 commits into from
Sep 2, 2021
Merged

Conversation

adamraine
Copy link
Member

Renders a lighthouse report in the flow report. I intentionally left ReportUIFeatures out for this PR.

@google-cla google-cla bot added the cla: yes label Aug 26, 2021
useLayoutEffect(() => {
if (!currentLhr) return;

dom.clearComponentCache();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this, style nodes are not added every time the LHR changes

let component = this._componentCache.get(componentName);
if (component) {
const cloned = /** @type {DocumentFragment} */ (component.cloneNode(true));
// Prevent duplicate styles in the DOM. After a template has been stamped
// for the first time, remove the clone's styles so they're not re-added.
this.findAll('style', cloned).forEach(style => style.remove());
return cloned;
}

flow-report/src/wrappers/report.tsx Show resolved Hide resolved
/**
* The jest environment "jsdom" does not work when preact is combined with the report renderer.
*/
export function setupJsDom() {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default jsdom test environment was not cooperating with the report renderer. These changes basically setup our own environment for testing with JSDOM.

The only problem I've encountered is that findBy* queries don't work, but we can just replace those with equivalent getBy* ones.

@@ -48,6 +48,21 @@ export function getScreenshot(reportResult: LH.ReportResult) {
return fullPageScreenshot || null;
}

export function convertChildAnchors(element: HTMLElement, index: number) {
const links = element.querySelectorAll('a') as NodeListOf<HTMLAnchorElement>;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

flow-report.d.ts can pull in the better typed querySelector and get this without the cast. report does it here:

// Import to augment querySelector/querySelectorAll with stricter type checking.
import '../../types/query-selector';

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This made the type errors visible in VSCode, but the type cast is still necessary. Either way I think it's a helpful change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

weird, I wonder if it's a js vs ts difference. Something to look into at some point

@adamraine adamraine marked this pull request as ready for review September 1, 2021 19:40
@adamraine adamraine requested a review from a team as a code owner September 1, 2021 19:40
@adamraine adamraine requested review from patrickhulce and removed request for a team September 1, 2021 19:40
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice! coming together nicely 👍

flow-report/assets/styles.css Outdated Show resolved Hide resolved
flow-report/src/util.ts Outdated Show resolved Hide resolved
flow-report/src/wrappers/report.tsx Outdated Show resolved Hide resolved
flow-report/src/wrappers/report.tsx Show resolved Hide resolved
export function useCurrentLhr(): {value: LH.Result, index: number}|null {
const flowResult = useFlowResult();
const [indexString, setIndexString] = useState(getHashParam('index'));
export function useHashParam(param: string) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@adamraine adamraine merged commit de9591a into master Sep 2, 2021
@adamraine adamraine deleted the flow-report-embedded-report branch September 2, 2021 14:56
satya-nutella pushed a commit to satya-nutella/lighthouse that referenced this pull request Sep 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants