core: add inline scripts to Scripts artifact

[connorjclark]

Fixes #7060.

I think it would be better to change the Scripts artifact to this shape:

{
  code: string
  requestId: string
}[]

I went with the lesser-diff change for the initial PR, but if the above seems like a good idea, I'll change it to that. thoughts?

[connorjclark]

now takes into account these two inline scripts

[brendankenny]

could we assert more in here to give insight about which 3 these should be? (shouldn't be exhaustive, but presumably the sort should be stable so could do urls)

[connorjclark]

done. the data was stable too so I include it.

[connorjclark]

PSA: typescript doesn't support type guarding for expressions like

const scriptContent = Array.isArray(artifacts.Scripts[requestId]) ?
        artifacts.Scripts[requestId] : [artifacts.Scripts[requestId]];

[patrickhulce]

If unminifed javascript is the only one we're impacting seems like it's worth just going whole hog to the structure you're proposing. We never arbitrarily access the script content by request id outside of the iteration it seems so, SGTM

[patrickhulce]

nice! thanks for picking that cleanup too @hoten !

[patrickhulce]

I'm a teensy bit worried that this is going to start throwing errors into our perf category on edge cases we previously didn't really care about. We don't actually need a request ID for the whole thing to work. WDYT about making requestId optional?

[connorjclark]

sounds alright, but what are these edge cases?

[patrickhulce]

If I knew them I would fix them :)

I just meant that we have seen this error popup in peoples reports before and it's mostly 🤷‍♂️and move on since it wasn't that big a deal. Now it'll be a bit more user-facing.

I actually had to tackle this for canonical though and think it'll be solid.

https://github.com/GoogleChrome/lighthouse/pull/7080/files#diff-2113a8215d43931339dbb50287d89dcbR450

[connorjclark]

done. thoughts on handling reporting of unminified scripts in the JS audit? I just put ? as a placeholder url :)

[connorjclark]

also, one of us should extract that function to somewhere else (whoever merges last)

[patrickhulce]

extract it off of network-analyzer? that actually seems like a good place for it :)

though if you mean we should finally move network-analyzer out of the depdency-graph/simulator folder then I agree with you :)

[patrickhulce]

done. thoughts on handling reporting of unminified scripts in the JS audit? I just put ? as a placeholder url

Ideally we'd try to provide a snippet of the code like we do for CSS, but I'm OK with this for now until we see how common it is.

[patrickhulce]

maybe when the fancy code snippet preview (#6901) lands we can do something cool here :D

[connorjclark]

yeah having it in network-analyzer is perfect

💯 to snippets

[patrickhulce]

update the artifacts.json?

[patrickhulce]

seems like this one isn't a meta ;)

[brendankenny]

maybe reverse these clauses, Note, HTML documents will have one entry per script tag, all with the same requestId.

[brendankenny]

this is expected?

[connorjclark]

nope- due to mistakenly changing the requestId values in Scripts w/o also changing the values in the devtools logs.

I assume I shouldn't just regenerate the whole thing, and instead minimize the diff by changing just what I needed to. Is that how we typically update sample_v2.json/artifacts.json?

[patrickhulce]

I usually edit it by hand, unless there's something really massive that we need to generate from a real run

[brendankenny]

what happened to zone.js? And it seems concerning that the thing we thought shouldn't ever happen (though in reality does happen very rarely), happens for our test case and so this falls back to '?' :)

What causes the main document not to be found in this case?

[connorjclark]

see above

[brendankenny]

why is this such a weird request id? Does the initial page now look like this? (vs the 1000024431.18 below that is the kind you usually see)

[connorjclark]

seems that the initial network record gets a different sort of ID than the records it later requests.

keep in mind that before, the inline scripts weren't in this Scripts object. So that's why you don't see the weird 44FDCEE5FC54A761B82815FAC6719910 request id previously.

[brendankenny]

since we're only interested in the one item, what about

assert.strictEqual(auditResult.items.length, 1);
const item = auditResult.items[0];
assert.strictEqual(item.url, '?');
assert.strictEqual(Math.round(item.wastedBytes / 1024), 98);
assert.strictEqual(Math.round(item.wastedPercent), 2);

(or with expect().toEqual() if that's preferred)

[brendankenny]

the other instances of finding the main resource mention URL.equalWithExcludedFragments being slow and so they have a quicker startsWith first pass. Seems like this should do the same if the speed is really a concern?

Agreed it'd be much preferable to call a centralized version of this.

[connorjclark]

done. after the two merge PRs merge maybe we should just move that check into URL.equalWithExcludedFragments as an early return

[patrickhulce]

maybe we should just move that check into URL.equalWithExcludedFragments as an early return

yeah it's a bit different to do generically because in these cases we know for a fact there are no fragments on the network records but agreed we should not have to do these things, the URL.* methods should just be fast by default :)

[brendankenny]

how do you feel about content instead of code? We use that in a few other places (and code is slightly overloaded)

[brendankenny]

seems like we always know the undefined case is the main page, so should it just say something like that here? (maybe with snippet eventually to identify which script in the main page is being analyzed)

[brendankenny]

seems like we always know the undefined case is the main page

admittedly we don't know that will always be the case, so maybe it's better to make that certain in the gatherer

[patrickhulce]

seems like we always know the undefined case is the main page

I think the problem here is inline scripts/evals added via other scripts. If it works like CSS those tend to be "inline" but shouldn't be considered part of the main page. I think the preference should be for code snippets showing the code itself, but I'd be more comfortable conveying it's in the "Other" / "Inline" / "Eval" bucket than the main page.

[brendankenny]

but I'd be more comfortable conveying it's in the "Other" / "Inline" / "Eval" bucket than the main page.

yeah, I was just thinking about those cases as well. Should we not search for a main resource at all then and always go for "Inline" or whatever? Seems like it will be common (a lot more common than a main resource not being found)

[patrickhulce]

Oh I gotcha, yeah agreed, we should just nix the finding of the main resource in the gatherer 👍

I think just putting inline without giving any idea to the content is pretty rough UX. Maybe we can at least show the first 40 characters or something?

[connorjclark]

I don't like omitting the requestId if it is readily available for inline content. How about not using an inline script's requestId in the audit, but keeping it in the artifact so that something else might use it if needed w/o grabbing the main record manually?

new pic:

[brendankenny]

should always be code, shouldn't there? Just excluding the zero-length case?

[connorjclark]

The new main doc scripts I added already have a zero-length check, so all good there. But the previous code was doing a zero-length check, and I didn't want to modify that behavior.

we could add a zero length check to the other scripts too, and then remove this check.

[brendankenny]

can we have a fallback like for url in computeWaste() since we know it's only scripts in the main page missing the network record?

[connorjclark]

seems that the initial network record gets a different sort of ID than the records it later requests.

keep in mind that before, the inline scripts weren't in this Scripts object. So that's why you don't see the weird 44FDCEE5FC54A761B82815FAC6719910 request id previously.

[connorjclark]

I shouldn't have changed these request id values. reverted.

[connorjclark]

nope- due to mistakenly changing the requestId values in Scripts w/o also changing the values in the devtools logs.

I assume I shouldn't just regenerate the whole thing, and instead minimize the diff by changing just what I needed to. Is that how we typically update sample_v2.json/artifacts.json?

[connorjclark]

see above

[connorjclark]

(fyi, did a first pass, will do second pass changes after a meeting)

[connorjclark]

Strangely, yarn smoke byte fails locally but not in appveyor

yarn smoke byte                                                                                            ✔  7201  13:46:19
yarn run v1.12.3
$ node lighthouse-cli/test/smokehouse/run-smoke.js byte
Running ONLY smoketests for: byte

Smoketest batch: perf-opportunity
byte smoketest starting…

byte smoketest results:
Command failed: node lighthouse-cli/test/smokehouse/smokehouse.js --config-path=lighthouse-cli/test/smokehouse/byte-config.js --expectations-path=byte-efficiency/expectations.js

Doing a run of 'http://localhost:10200/byte-efficiency/tester.html'...
$ node lighthouse-cli/index.js http://localhost:10200/byte-efficiency/tester.html --config-path=/Users/cjamcl/src/lighthouse/lighthouse-cli/test/smokehouse/byte-config.js --output-path=smokehouse-10774.report.json --output=json --quiet --port=0
Asserting expected results match those found. (http://localhost:10200/byte-efficiency/tester.html)
  ✓ final url: http://localhost:10200/byte-efficiency/tester.html
  ✓ error code: undefined

  ✘ difference at unused-javascript.details.overallSavingsBytes
              expected: ">=25000"
                 found: 21018

          found result:
      {
        "id": "unused-javascript",
        "title": "Remove unused JavaScript",
        "description": "Remove unused JavaScript to reduce bytes consumed by network activity.",
        "score": 0.75,
        "scoreDisplayMode": "numeric",
        "rawValue": 300,
        "displayValue": "Potential savings of 21 KB",
        "details": {
          "type": "opportunity",
          "headings": [
            {
              "key": "url",
              "valueType": "url",
              "label": "URL"
            },
            {
              "key": "totalBytes",
              "valueType": "bytes",
              "label": "Size (KB)"
            },
            {
              "key": "wastedBytes",
              "valueType": "bytes",
              "label": "Potential Savings (KB)"
            }
          ],
          "items": [
            {
              "url": "http://localhost:10200/byte-efficiency/script.js",
              "totalBytes": 53181,
              "wastedBytes": 14415,
              "wastedPercent": 27.105307847614018
            },
            {
              "url": "http://localhost:10200/byte-efficiency/tester.html",
              "totalBytes": 15073,
              "wastedBytes": 6603,
              "wastedPercent": 43.80430423740142
            }
          ],
          "overallSavingsMs": 300,
          "overallSavingsBytes": 21018
        }
      }


  ✘ difference at offscreen-images.details.items[3].url
              expected: "/large.svg$/"
                 found: "http://localhost:10200/byte-efficiency/lighthouse-480x320.webp?lazilyLoaded=true"

          found result:
      {
        "id": "offscreen-images",
        "title": "Defer offscreen images",
        "description": "Consider lazy-loading offscreen and hidden images after all critical resources have finished loading to lower time to interactive. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/offscreen-images).",
        "score": 1,
        "scoreDisplayMode": "numeric",
        "rawValue": 0,
        "displayValue": "Potential savings of 88 KB",
        "warnings": [],
        "details": {
          "type": "opportunity",
          "headings": [
            {
              "key": "url",
              "valueType": "thumbnail",
              "label": ""
            },
            {
              "key": "url",
              "valueType": "url",
              "label": "URL"
            },
            {
              "key": "totalBytes",
              "valueType": "bytes",
              "label": "Size (KB)"
            },
            {
              "key": "wastedBytes",
              "valueType": "bytes",
              "label": "Potential Savings (KB)"
            }
          ],
          "items": [
            {
              "url": "http://localhost:10200/byte-efficiency/lighthouse-unoptimized.jpg",
              "requestStartTime": 98037.281547,
              "totalBytes": 51941,
              "wastedBytes": 51941,
              "wastedPercent": 100
            },
            {
              "url": "http://localhost:10200/byte-efficiency/lighthouse-480x320.webp",
              "requestStartTime": 98039.276147,
              "totalBytes": 11460,
              "wastedBytes": 11460,
              "wastedPercent": 100
            },
            {
              "url": "http://localhost:10200/byte-efficiency/lighthouse-480x320.webp?invisible",
              "requestStartTime": 98039.286467,
              "totalBytes": 11460,
              "wastedBytes": 11460,
              "wastedPercent": 100
            },
            {
              "url": "http://localhost:10200/byte-efficiency/lighthouse-480x320.webp?lazilyLoaded=true",
              "requestStartTime": 98045.118763,
              "totalBytes": 11460,
              "wastedBytes": 11460,
              "wastedPercent": 100
            },
            {
              "url": "http://localhost:10200/byte-efficiency/large.svg",
              "requestStartTime": 98039.743435,
              "totalBytes": 3281,
              "wastedBytes": 3281,
              "wastedPercent": 100
            }
          ],
          "overallSavingsMs": 0,
          "overallSavingsBytes": 89602
        }
      }

  Correctly passed 7 assertions

7 passing
2 failing
smoketest-byte: 32674.869ms
byte smoketest complete.

We have 1 failing smoketests: byte
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[connorjclark]

The above only happens in canary, not stable. ¯_(ツ)_/¯

[connorjclark]

as a sanity check I tried setting breakpoints in the "used" unused function. I suppose breakpoints on code that v8 determines is dead code is not possible?

impossible to set a breakpoint there. (also, in the if (false) block). I have seen this behavior before but never connected it to dead code, just thought devtools was buggy :)

[connorjclark]

https://bugs.chromium.org/p/chromium/issues/detail?id=927464

[brendankenny]

https://bugs.chromium.org/p/chromium/issues/detail?id=927464

we're not blocked on this, though, right? Since it affects things regardless of the shape of the artifact?

[connorjclark]

correct, that test fails locally on canary regardless of any changes.

[connorjclark]

Reference image:

We could also link to the script tag, but that seems like an enhancement that can be punted. Perhaps would require adding a nodePath to the Scripts artifact?

[connorjclark]

Do we consider changing an artifact to be a breaking change?

[patrickhulce]

Do we consider changing an artifact to be a breaking change?

I think only after 5.0 when we make plugins an official public thing

[patrickhulce]

I would feel slightly better about a looser assertion here

[connorjclark]

wb a new syntax like x % 5 to indicate within +/- 5% of x?

[patrickhulce]

Oooh I love that, how about x +/- y though ;)

[patrickhulce]

+/- landed right we can update these now?

[patrickhulce]

Oh I gotcha, yeah agreed, we should just nix the finding of the main resource in the gatherer 👍

I think just putting inline without giving any idea to the content is pretty rough UX. Maybe we can at least show the first 40 characters or something?

[brendankenny]

+/-

can you move to a separate PR :)

[connorjclark]

+/-

can you move to a separate PR :)

sure. stashing this here for now:

items: [
    {
      url: 'http://localhost:10200/byte-efficiency/script.js',
      wastedBytes: '46481 +/- 100',
      wastedPercent: '87 +/- 5',
    },
    {
      url: 'inline: \n  function unusedFunction() {\n    // Un...',
      wastedBytes: '6581 +/- 100',
      wastedPercent: '99.6 +/- 0.1',
    },
    {
      url: 'inline: \n  // Used block #1\n  // FILLER DATA JUS...',
      wastedBytes: '6559 +/- 100',
      wastedPercent: 100,
    },
  ],

[brendankenny]

update for the +/- and LGTM!

I'll leave it to @patrickhulce for any remaining particulars in byte efficiency or whatever else

[brendankenny]

rather than fall into the boolean trap, what about making a @param {string} displayUrl (or whatever) parameter and doing the url/inline ternary down in audit_ before calling this?

[brendankenny]

how many places do we figure this out now? :)

[brendankenny]

move this out of the map to a named variable to keep it cleaner in here.

It definitely sucks that we have to add this for the case that should never happen but turns out does. Maybe add a comment too about mainResource is expected to be found, but on the very small chance something weird happens and it isn't found, we'll roll without it for now?

[brendankenny]

oh, actually, forgot about the earlier conversation. We should switch to NetworkAnalyzer.findMainDocument() once #7080 lands so we can make requestId not optional

[patrickhulce]

A few minor things and +1s to brendan's comments, but LGTM overall!

[patrickhulce]

+/- landed right we can update these now?

[patrickhulce]

why did this change?

[connorjclark]

whitespace changes in the provided network record contents. the content: eats up some of the ws :)

[patrickhulce]

same here

[patrickhulce]

super nit: it feels like the inline is the special case we need to check for so it reads a bit easier in my head to say

an inline thing is something that's explicitly marked as such or doesn't have a network record
inline things get a prefix and content while everything else has a URL like normal

[patrickhulce]

I would say "the networkRecord requestId that contained their content" since for it very well could have been a different script that "loaded" that other script.

[patrickhulce]

LGTM