misc: specify types for some dependency-graph options objects #7962
Conversation
|
|
||
| /** | ||
| * @typedef NetworkAnalyzer.RTTEstimateOptions | ||
| * @property {boolean} [forceCoarseEstimates] TCP connection handshake information will be used when available, but for testing it's useful to see how the coarse estimates compare with higher fidelity data. |
There was a problem hiding this comment.
I don't believe the emphasis on "for testing it's useful..." is entirely true anymore (ConnectionPool calls NetworkAnalyzer.estimateIfConnectionWasReused with forceCoarseEstimates: true), but I don't know enough to update this :)
There was a problem hiding this comment.
Oh yeah we needed to turn it on over there because the connection id information just lied so often in certain enviornments so it was just easier to always ignore it.
Probably worth rewording to something like
TCP connection handshake information will be used when available, but in some circumstances this data can be unreliable. This flag exposes an option to ignore the handshake data and use the coarse download/TTFB timing data.
?
| @@ -61,4 +61,12 @@ describe('DependencyGraph/Simulator/DNSCache', () => { | |||
| expect(dns.getTimeUntilResolution(request, {requestedAt: 2000})).toBe(0); | |||
| }); | |||
| }); | |||
|
|
|||
| describe('.setResolvedAt', () => { | |||
There was a problem hiding this comment.
coverage low hanging fruit :)
| ); | ||
| const { | ||
| forceCoarseEstimates = false, | ||
| coarseEstimateMultiplier = 0.3, |
There was a problem hiding this comment.
FWIW, I think we lose a little value by moving the explanation away from the default on this one
There was a problem hiding this comment.
FWIW, I think we lose a little value by moving the explanation away from the default on this one
Yeah, sounds good. We should maybe move it to a top-level constant (with the comment), but just moved the comment back into place for now
|
|
||
| /** | ||
| * @typedef NetworkAnalyzer.RTTEstimateOptions | ||
| * @property {boolean} [forceCoarseEstimates] TCP connection handshake information will be used when available, but for testing it's useful to see how the coarse estimates compare with higher fidelity data. |
There was a problem hiding this comment.
Oh yeah we needed to turn it on over there because the connection id information just lied so often in certain enviornments so it was just easier to always ignore it.
Probably worth rewording to something like
TCP connection handshake information will be used when available, but in some circumstances this data can be unreliable. This flag exposes an option to ignore the handshake data and use the coarse download/TTFB timing data.
?
No issues, just for documentation and ease of cross referencing.
Where the only caller(s) passed in definitely defined arguments, also made the properties required (I think this only happened for the
ConnectionPoolandDNSCacheconstructors).