-
Notifications
You must be signed in to change notification settings - Fork 15
Conversation
@kasperisager whats the preupgradeLength needed for here? |
@mafintosh It's returned from |
and we can't just use the snapshot mechanic for that always? |
@mafintosh We can totally do that, that should simplify things a tad 👍 |
} | ||
|
||
onupgrade (request) { | ||
if (this.active) this.active.cancel() |
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 ensures that only 1 upgrade is active at a time by cancelling previous upgrades as new ones are applied. Not 100% sure if that's what we're after though.
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.
either this, or it "debounces", ie reruns the hook
|
||
if (this.upgrader.preupgrade) session._updateSnapshot() | ||
|
||
let upgraded = await request |
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.
whats stopping us from awating the request in update itself? seems cleaner
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.
Need to do session._updateSnapshot()
before the update is applied to the core to keep the session on the preupgrade length.
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.
Is session here, the session the user called .update on?
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.
Yep (this
):
Line 83 in 94e4e3a
this._upgrader = new Upgrader(this, opts) |
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.
Ok, will see what we can do to improve this, but ok for now.
@kasperisager sorry to do this, but can you move this to hypercore-protocol/hypercore ? |
@mafintosh No problem at all! Rebased onto latest |
Closes #128