WIP: add snapshot support to ScriptElements #12770
Conversation
patrickhulce
left a comment
patrickhulce
left a comment
There was a problem hiding this comment.
Nice work!
but we'll need to add the requestId separately kinda like what ImageRecords does in #12663.
FWIW, I think this is a positive thing to do regardless of a split. Aligns our artifacts as unique bits of data :)
we need to fetch the contents over the protocol using Debugger.getScriptSource instead of looking at the network request
Does this work in our current gatherer in most cases? If so, I'm inclined to not maintain a fork. I don't know the answer to this but I might suspect that the network cache contents fetching we do is backed by the same data source as the Debugger version 🤷
The main use case for ScriptElementsSnapshot would be a new audit unminified-javascript-snapshot, a snapshot version of unminified-javascript.
Is this really it? If we're not able to convert any of the other JS opportunities as well, I'm also more hesitant to maintain a copy.
| const session = context.driver.defaultSession; | ||
| const executionContext = context.driver.executionContext; | ||
|
|
||
| if (context.gatherMode === 'snapshot') { |
There was a problem hiding this comment.
it looks like you wrote this originally as a merged version, amirite? :)
It should. I didn't test it in navigation or timespan mode though.
That's what's possible with this change alone. I will need to reinvestigate some audits like |
With a few changes, navigation and timespan are supported in the currently named |
Great! I'm a little confused though, seems like we don't need the separate gatherer in that case then? If so, what feedback are you blocked on from reviewer? :) |
The new gatherer cannot provide the |
Ah, yes it technically would. I couldn't find any usages with a quick GitHub, so we could consider returning Should we shelve this until we can make a few 9.0 breaking changes? |
It's used in
I'm ok with shelving, adding snapshot support here isn't mission critical. |
I meant usages by other open source projects ;) we can definitely fix our own usages without breaking
Sounds good! |
|
We will continue this post-9.0, but probably keep existing |
4 participants
To get script contents in snapshot mode, we need to fetch the contents over the protocol using
Debugger.getScriptSourceinstead of looking at the network request. Unfortunatley, we can't get therequestIdthis way.We can replace
ScriptElementswith the implementation inScriptElementsSnapshotfor timespan and navigation modes, but we'll need to add therequestIdseparately kinda like whatImageRecordsdoes in #12663. SinceScriptElementsandScriptElementsSnapshotare so different, I'm inclined to make that a breaking change and keep two separate gatherers for now.The main use case for
ScriptElementsSnapshotwould be a new auditunminified-javascript-snapshot, a snapshot version ofunminified-javascript.