core(noopener): use node detail type #10242

Beytoven · 2020-01-21T22:28:47Z

Summary

We want a better UX for viewing the noopener audit's results in devtools.

This does change the audit's result shape which is a breaking change.

Related Issues/PRs

connorjclark · 2020-01-21T23:01:51Z

lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js

          href: anchor.href || 'Unknown',
          target: anchor.target || '',
          rel: anchor.rel || '',
          outerHTML: anchor.outerHTML || '',
        };
+        return item;


failingAnchors -> items

connorjclark · 2020-01-21T23:02:51Z

lighthouse-core/gather/gatherers/anchor-elements.js


    if (node instanceof HTMLAnchorElement) {
      return {
        href: node.href,
+        // TODO(blasingame): Deprecate text in favor of nodeLabel


6.0 is coming up, we should do this now.

I'm not totally convinced we want to remove this. The purpose of including some of these properties was so that plugins could take advantage of generic information that is of common interest and untruncated text seems like something useful to surface. The alt text feature of getNodeLabel is pretty nice though.

I'd say we can resolve at a later date, but @connorjclark is right that 6.0 is imminent.

Options I see, but feel free to add:

Add nodeLabel and keep text

Remove text and keep nodeLabel

Change text to just be output of getNodeLabel

Make truncateLength a parameter to getNodeLabel and use it as text without truncation

I'm a fan of 1 and 4, personally. Thoughts anyone?

yah option 1 works for me. thanks for thinking it through

I've removed the TODO as option 1 is already taken care of as part of this PR.

patrickhulce

nice work @Beytoven! heads up it's gonna need a yarn update:sample-json to fix the CI tests :)

patrickhulce · 2020-01-22T01:55:59Z

lighthouse-core/audits/dobetterweb/external-anchors-use-rel-noopener.js

-  /** Label for a column in a data table; entries will be the values of the html "rel" attribute from link in a page.  */
-  columnRel: 'Rel',
+  /** Label for a column in a data table; entries will be the HTML elements that failed the audit. Anchors are DOM elements that are links. */
+  failingAnchorsHeader: 'Failing Anchors',


nit: when text is exclusively used for a column label we tend to follow the columnNameOfThing pattern

patrickhulce · 2020-01-22T02:16:37Z

lighthouse-core/gather/gatherers/anchor-elements.js


    if (node instanceof HTMLAnchorElement) {
      return {
        href: node.href,
+        // TODO(blasingame): Deprecate text in favor of nodeLabel


I'm not totally convinced we want to remove this. The purpose of including some of these properties was so that plugins could take advantage of generic information that is of common interest and untruncated text seems like something useful to surface. The alt text feature of getNodeLabel is pretty nice though.

I'd say we can resolve at a later date, but @connorjclark is right that 6.0 is imminent.

Options I see, but feel free to add:

Add nodeLabel and keep text

Remove text and keep nodeLabel

Change text to just be output of getNodeLabel

Make truncateLength a parameter to getNodeLabel and use it as text without truncation

I'm a fan of 1 and 4, personally. Thoughts anyone?

patrickhulce · 2020-01-22T02:18:58Z

lighthouse-core/test/audits/dobetterweb/external-anchors-use-rel-noopener-test.js

@@ -88,19 +84,16 @@ describe('External anchors use rel="noopener"', () => {
    });
    assert.equal(auditResult.score, 0);
    assert.equal(auditResult.details.items.length, 2);
-    assert.equal(auditResult.details.items.length, 2);
+    assert.equal(auditResult.details.items[0].node.type, 'node');


maybe we could get at least 1 assertion on the structure of the node value?

I added the devtoolsNodePath to the AnchorElement mocks and added a couple assertions on the resulting item's node path. Not sure if this is what you meant though.

patrickhulce · 2020-01-22T02:19:21Z

types/artifacts.d.ts

@@ -289,6 +289,9 @@ declare global {
        href: string
        text: string
        target: string
+        path: string


in scripts gatherer (one of the other "public" gatherers) we named this devtoolsNodePath to make it clear this isn't for general use like the selector.

Rename failingAnchorsHeader to columnFailingAnchors to be in line with established pattern. Add assertions on the value of devtoolNodePath to unit tests.

…nd return it directly in map function

…tched to null

patrickhulce

LGTM! 🎉

connorjclark

LGTM 🎉

connorjclark · 2020-01-23T22:21:20Z

BTW, I noticed you're adding some really descriptive messages for the commits here. Just so ya know, we squash all PRs and generally don't include a commit body (just the title of the PR), unless it's a rather complex change that warrants more. As long as the PR title is a well crafted commit message, that's enough.

Of course, you should still be descriptive in each commit if you like. It's probably helpful for reviewers. But not 100% necessary imo.

paulirish · 2020-01-25T00:24:36Z

BTW, I noticed you're adding some really descriptive messages for the commits here.

Yup, mostly what connor said. 👍 good call

the PR title is key and it's populated from the first commit message.. personally i try to craft that one right, but you can obv make the PR title whatever before you send it out.

as for all subsequent commits, yeah... often we're very lazy and use 'feedback' or 'json' or 'tests', etc. you can feel free to stay descriptive if its helpful for you. but it's true that it wont be seen in the master branch's history. ;)

core(noopener): use itemType node for noopener audit

6005a0d

Beytoven requested review from brendankenny, patrickhulce and paulirish as code owners January 21, 2020 22:28

googlebot added the cla: yes label Jan 21, 2020

connorjclark requested changes Jan 21, 2020

View reviewed changes

patrickhulce suggested changes Jan 22, 2020

View reviewed changes

Rename path in AnchorElement to devtoolNodePath to be more descriptive.

40e3f1b

Rename failingAnchorsHeader to columnFailingAnchors to be in line with established pattern. Add assertions on the value of devtoolNodePath to unit tests.

vercel bot deployed to Preview January 22, 2020 21:09 View deployment

Remove redundant step of assigning failingAnchors obj to a variable a…

639c3e4

…nd return it directly in map function

vercel bot temporarily deployed to Preview January 22, 2020 21:14 Inactive

Ran yarn update:sample-json

199c63e

Beytoven requested a review from exterkamp as a code owner January 22, 2020 21:16

vercel bot deployed to Preview January 22, 2020 21:16 View deployment

Fix lint errors

268e3d0

vercel bot deployed to Preview January 22, 2020 22:06 View deployment

ran yarn update:sample-json again

cbe6fd1

vercel bot deployed to Preview January 22, 2020 22:27 View deployment

Manually revert protobuff sample changes where empty objects were swi…

4a32b15

…tched to null

vercel bot deployed to Preview January 22, 2020 22:59 View deployment

Remove TODO as we'll keep both text and nodeLabel

c1c8203

vercel bot deployed to Preview January 22, 2020 23:03 View deployment

patrickhulce approved these changes Jan 23, 2020

View reviewed changes

connorjclark changed the title ~~core(noopener): use itemType node for noopener audit~~ core(noopener): use node detail type Jan 23, 2020

connorjclark approved these changes Jan 23, 2020

View reviewed changes

connorjclark merged commit 1e9f2f9 into master Jan 23, 2020

connorjclark deleted the noopener branch January 23, 2020 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(noopener): use node detail type #10242

core(noopener): use node detail type #10242

Beytoven commented Jan 21, 2020

connorjclark Jan 21, 2020

connorjclark Jan 21, 2020

patrickhulce Jan 22, 2020

paulirish Jan 22, 2020

Beytoven Jan 22, 2020

patrickhulce left a comment

patrickhulce Jan 22, 2020

patrickhulce Jan 22, 2020

patrickhulce Jan 22, 2020

Beytoven Jan 22, 2020

patrickhulce Jan 22, 2020

patrickhulce left a comment

connorjclark left a comment •

edited

Loading

connorjclark commented Jan 23, 2020 •

edited

Loading

paulirish commented Jan 25, 2020

core(noopener): use node detail type #10242

core(noopener): use node detail type #10242

Conversation

Beytoven commented Jan 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

connorjclark left a comment • edited Loading

Choose a reason for hiding this comment

connorjclark commented Jan 23, 2020 • edited Loading

paulirish commented Jan 25, 2020

connorjclark left a comment •

edited

Loading

connorjclark commented Jan 23, 2020 •

edited

Loading