-
Notifications
You must be signed in to change notification settings - Fork 49
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
add timestamp to handshake #183
Conversation
relayKeepAlive: opts.relayKeepAlive || 5000, | ||
|
||
// Used by the server when tie-breaking between two connections from the same peer | ||
timestamp: opts.timestamp ?? -1 |
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.
this default of -1
makes the timestamp behavior opt in... which may be a pain-in-the-butt for most users, who'll not benefit from this change until they modify their code to pass { timestamp: Date.now() }
in the options to connect
.
However, I tend to be very conservative when it comes to breaking changes, and my philosophy is usually to make all enhancements "opt in" until the next major version bump, at which time I update the defaults.
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.
just use 0 as the default, then the uint can be unsigned, and 0 is pretty appropriate for that. also means we can just do || 0
Closed in favor of holepunchto/hyperswarm#178 |
Supercedes #167 . It's better because:
timestamp
is monotonically increasing ¹ ²hyperswarm
¹
Date.now()
is not guaranteed to be monotonically increasing, but since fast reconnects are not mission critical, it should suffice, since it'll succeed "all" the time for practical purposes.² According to MDN,
performance.timeOrigin + performance.now()
is guaranteed to be monotonically increasing. But I don't think theperformance
global is available inbare
JS environments.