-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ndt7-js][typescript] add type declaration file #24
base: main
Are you sure you want to change the base?
Conversation
}; | ||
|
||
interface ServerData { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@robertodauria what are the expected types for ServerData
?
src/ndt7.d.ts
Outdated
LastServerMeasurement: ServerData; | ||
} | ||
|
||
export function discoverServerURLs(config: DiscoverServerURLsConfig, userCallbacks: DiscoverServerURLsCallbacks) : Promise<URLs>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are config
and userCallbacks
optional parameters? if so, they need to follow required parameters (in this case, urlPromise
)
TS error: A required parameter cannot follow an optional parameter
} | ||
|
||
interface Config { | ||
userAcceptedDataPolicy?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should all configurations that extend Config
contain these two fields?
} | ||
|
||
interface DiscoverServerURLsCallbacks extends Callbacks { | ||
serverDiscovery?: (options: { loadbalancer: string | URL }) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I change this to loadbalancerURL
to avoid confusion with loadbalancer
in DiscoverServerURLsConfig
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @aileen15, thank you for this PR and sorry for being so late!
I replied to your questions below, but please let me know if you prefer I make the changes directly 🙂
Reviewable status: 0 of 1 approvals obtained (waiting on @aileen15)
src/ndt7.d.ts, line 17 at r1 (raw file):
Previously, aileen15 wrote…
should all configurations that extend
Config
contain these two fields?
Yes, we would like integrators to always ask the user to read and accept the data policy (and report it to the library via this flag) or say this is not applicable (e.g. for private usage of the library).
src/ndt7.d.ts, line 40 at r1 (raw file):
Previously, aileen15 wrote…
should I change this to
loadbalancerURL
to avoid confusion withloadbalancer
inDiscoverServerURLsConfig
?
Sounds good!
src/ndt7.d.ts, line 69 at r1 (raw file):
Previously, aileen15 wrote…
@robertodauria what are the expected types for
ServerData
?
It's an object containing three sub-objects:
- BBRInfo (all numbers - see https://github.com/m-lab/tcp-info/blob/master/inetdiag/structs.go#L376)
- ConnectionInfo (all strings - see https://github.com/m-lab/ndt-server/blob/3d30b2a84bc2f3251d8a72b68259de3d309f9a78/ndt7/model/archivaldata.go#L41)
- TCPInfo (all numbers - see https://github.com/m-lab/tcp-info/blob/master/tcp/tcpinfo.go#L58)
Example
BBRInfo:
BW: 9432964
CwndGain: 739
ElapsedTime: 284944
MinRTT: 37044
PacingGain: 739
--
ConnectionInfo:
Client: "109.234.59.182:34896"
Server: "91.239.96.88:443"
UUID: "ndt-lhpxc_1648245098_00000000000B1527"
--
TCPInfo:
ATO: 40000
AdvMSS: 1448
AppLimited: 0
Backoff: 0
BusyTime: 328000
BytesAcked: 773388
BytesReceived: 1561
BytesRetrans: 0
BytesSent: 1546668
CAState: 0
DSackDups: 0
DataSegsIn: 3
DataSegsOut: 1076
Delivered: 540
DeliveredCE: 0
DeliveryRate: 9433000
ElapsedTime: 284944
Fackets: 0
LastAckRecv: 4
LastAckSent: 0
LastDataRecv: 288
LastDataSent: 4
Lost: 0
MaxPacingRate: -1
MinRTT: 37044
NotsentBytes: 2630880
Options: 7
PMTU: 1500
PacingRate: 26958011
Probes: 0
RTO: 240000
RTT: 39027
RTTVar: 409
RWndLimited: 0
RcvMSS: 937
RcvOooPack: 0
RcvRTT: 45000
RcvSpace: 14600
RcvSsThresh: 64076
ReordSeen: 0
Reordering: 3
Retrans: 0
Retransmits: 0
Sacked: 0
SegsIn: 225
SegsOut: 1079
SndBufLimited: 0
SndCwnd: 546
SndMSS: 1440
SndSsThresh: 2147483647
SndWnd: 1277696
State: 1
TotalRetrans: 0
Unacked: 537
WScale: 119
src/ndt7.d.ts, line 77 at r1 (raw file):
Previously, aileen15 wrote…
are
config
anduserCallbacks
optional parameters? if so, they need to follow required parameters (in this case,urlPromise
)TS error:
A required parameter cannot follow an optional parameter
config
should be mandatory since it contains mandatory fields (either the user accepted the privacy policy, or the privacy policy is inapplicable)
userCallbacks
can in theory not be mandatory, but the default callback does not do anything useful. I would make it mandatory to make it clear users are expected to override the callbacks. Thoughts?
Changes
ndt7.js
types
field inpackage.json
ndt7-js
node moduleReviewers
@robertodauria
Resources
This change is