core: add path property to script elements artifact #8133
Conversation
| @@ -9,6 +9,9 @@ const Gatherer = require('./gatherer.js'); | |||
| const NetworkAnalyzer = require('../../lib/dependency-graph/simulator/network-analyzer.js'); | |||
| const NetworkRequest = require('../../lib/network-request.js'); | |||
| const getElementsInDocumentString = require('../../lib/page-functions.js').getElementsInDocumentString; // eslint-disable-line max-len | |||
| const pageFunctions = require('../../lib/page-functions.js'); | |||
|
|
|||
| /* global getNodePath */ | |||
|
|
|||
| /** | |||
| * @return {LH.Artifacts['ScriptElements']} | |||
There was a problem hiding this comment.
we need to update the types too
| @@ -26,6 +29,8 @@ function collectAllScriptElements() { | |||
| async: script.async, | |||
| defer: script.defer, | |||
| source: /** @type {'head'|'body'} */ (script.closest('head') ? 'head' : 'body'), | |||
| // @ts-ignore - getNodePath put into scope via stringification | |||
| path: getNodePath(script), | |||
There was a problem hiding this comment.
path might be a tad too generic on the *Elements artifacts for my taste, given it's basically a devtools-specific crazy node child index array
WDYT about...
devtoolsNodePath
devtoolsChildIndexNodePath
nodePathForDevToolsRevealing
any other suggestions welcome :)
There was a problem hiding this comment.
nodeReferencePath? I'd like to avoid tying it to DevTools, since other consumers could theoretically use it too. Unless we strongly think they shouldn't.
So for now we'd use the new name here and keep path everywhere else?
There was a problem hiding this comment.
since other consumers could theoretically use it too
😱 I hope not!
😆 If you can't tell, I'm not fond of this particular path approach, so I would prefer to limit its scope and push a more flexible strategy that can be resilient to reloads.
So for now we'd use the new name here and keep path everywhere else?
yeah just thinking out loud here. I've proposed to only expose the *Elements artifacts as the public API for v5 so I'm thinking of them quite separately from the others at this point, which might not align with the rest of LH team yet :)
There was a problem hiding this comment.
I would prefer to limit its scope and push a more flexible strategy that can be resilient to reloads.
#2289 (comment) fyi
There was a problem hiding this comment.
Thanks @hoten I'm glad we've already made some awesome improvements here :D
To clarify, my main goal if we're exposing this to plugin consumers in *Elements would be to provide a path that has some value even when the node cannot be found, i.e. based on some sort of identifiers/classes/etc on the page and not just child indexes that don't provide much value to a human, some selector/path hybrid I guess.
There was a problem hiding this comment.
😱 I hope not!
Ok, __legacyDevToolsPathDontUseEver it is then 😂
some selector/path hybrid I guess
Were you thinking of the selector property we show currently when the user hovers over a node in the report?
Axe already aims to make that a unique selector. getNodeSelector generates selectors based on the id/class name if available, but doesn't attempt to make that unique.
Seems like we don't need path at all if we can make selector unique.
There was a problem hiding this comment.
__legacyDevToolsPathDontUseEver 😍 love it! 😆
I don't want to hold things up on this, but I'm just afraid of what we won't be able to take back once 5.0 hits :)
Anyone else core have thoughts on our public artifacts API? I think I might stir up this convo over on the issue...
There was a problem hiding this comment.
devtoolsNodePath sounds great to me, personally. Sounds uninvitingly opaque if you aren't devtools, you still know exactly what it is, and even if you do figure out how to use it, it sounds like the gears of devtools turning could change the format at any time.
Also fine with a leading underscore (but I vote we stay away from two :) and a strongly worded comment, but I think that's more than enough at that point.
There was a problem hiding this comment.
@paulirish if he has an opinion
There was a problem hiding this comment.
fine with devtoolsNodePath. seems cool.
Here's where the protocol uses it: https://vanilla.aslushnikov.com/?DOM.pushNodeByPathToFrontend
|
travis is harder to please though :) I think you'll have to provide an empty one for the network-discovered scripts |
Summary
When displaying JSON-LD script elements in DevTools we want to link them to the Elements panel, so we need to collect the node
pathto do that.I saw the ScriptElements PR also updated
lighthouse-core/test/results/artifacts/artifacts.json, but I'm not sure how that's updated...