core(lantern): add interface for network request #15845
Conversation
This reverts commit 25471ba.
This mean all our usages of Lantern (using the records from the graph) will need to be updated to |
| */ | ||
| get record() { | ||
| return this._record; | ||
| return /** @type {T} */ (this._request.record); |
There was a problem hiding this comment.
nit: I wouldn't expect this type cast to be necessary, are there errors if it's removed?
There was a problem hiding this comment.
This removes undefined from the type from .record, which is that way to make our NetworkRequest assignable to Lantern.NetworkRequest. redirectDestination: NetworkRequest<T> is the issue. Resolving this won't be trivial, and should be done in a separate PR if at all.
| } | ||
|
|
||
| /** | ||
| * @return {LH.Artifacts.NetworkRequest} | ||
| * @return {T} | ||
| */ | ||
| get record() { |
There was a problem hiding this comment.
I think it's kinda confusing to have record and request properties. Could we rename record to internalRecord/recordImpl or something to emphasize which one is the normal use case and which one is for debugging.
There was a problem hiding this comment.
I kept record in this PR just to prevent a big name churn in our usage of Lantern. We can rename them, in another PR to keep this one clearer. I don't know what good names would be right now.
| @@ -7,7 +7,8 @@ | |||
| /** | |||
| * A union of all types derived from BaseNode, allowing type check discrimination | |||
| * based on `node.type`. If a new node type is created, it should be added here. | |||
| * @typedef {import('./cpu-node.js').CPUNode | import('./network-node.js').NetworkNode} Node | |||
| * @template [T=any] | |||
| * @typedef {import('./cpu-node.js').CPUNode<T> | import('./network-node.js').NetworkNode<T>} Node | |||
There was a problem hiding this comment.
Assuming CPUNode does eventually need a generic, this type asserts that T will be the same between NetworkNode and CPUNode which would never be the case. Maybe we should just remove the generic from CPUNode for now?
There was a problem hiding this comment.
Maybe we should just remove the generic from CPUNode for now?
This is a requirement for type narrowing to continue to work.
// in graph.traverse callback...
if (node.type !== 'network') return;
// node.record should be NetworkRequest
if (!CriticalRequestChains.isCritical(node.record, mainResource)) return;
Remove the type parameter from BaseNode, CPUNode, or Node, and in the above node.record is any.
This reverts commit 06b2125.
| }; | ||
| export class NetworkRequest<T=any> { | ||
| /** The canonical network record. */ | ||
| record?: T; |
There was a problem hiding this comment.
Can you explain why this is necessary? I thought the point of the interface was to accept the LH NetworkRequest as is?
(to be clear, I haven't played with the types at all and haven't really been following along, just curious)
There was a problem hiding this comment.
Added a comment.
You can assign our NetworkRequest to Lantern.NetworkRequest, since this property is optional.
We have a different source of truth for RPP's trace/synthetic network record events. The shape does overlap with our NetworkRequest, but we have more stuff on our NetworkRequest such that we want to keep node.record for NetworkNode.
An idea is to remove T from everything but NetworkNode, but then callers of Node.traverse (and other functions) would need to provide T themselves. So we'd have to jsdoc type the parameters of every use case. The alternative is what the PR is currently doing, which is to have T on BaseNode, which forces CPUNode to come along for the ride. It's not ideal, and maybe it can be addressed in a better way later.
ref #15841
alternative to #15842
I grabbed the generated
.d.tsfor network-request.js and removed the static methods and other things not relevant for usage within Lantern.