Conversation
jeswr
left a comment
jeswr
left a comment
There was a problem hiding this comment.
Only the documentation comment is directly relevant to this PR. The rest can be applied separately in follow ups.
There was a problem hiding this comment.
It looks like NodeJS support for the Web Lock API was added in Node 24. Node 22 is in maintenance until April 2027 - we should therefore document that Node v24 is the minimum supported node version.
I'd also double check they haven't backported it before writing the exact version.
There was a problem hiding this comment.
Good point, thx.
| const {promise: responseFromPopup, reject: cancelCodeRequest, resolve: respondWithCode} = Promise.withResolvers<string>() | ||
| signal.throwIfAborted() |
There was a problem hiding this comment.
Why not throw before creating the promise.
| this.#dialog.addEventListener("close", onClose, onlyOnce) | ||
|
|
||
| return await promise | ||
| return await promise |
There was a problem hiding this comment.
| return await promise | |
| return promise |
There was a problem hiding this comment.
I would rather not.
Really, both asyncs and both awaits in this method could be eliminated because nothing in the method itself uses the value of the promises.
But instead I choose to spell out both pairs because it seems clearer to me what is happening. In general I prefer to await all promises rather than rely on the reader to untangle them.
| this.#dialog.addEventListener("close", onClose, onlyOnce) | ||
|
|
||
| return await promise | ||
| return await promise |
There was a problem hiding this comment.
| return await promise | |
| return promise |
| // One flow at a time, fellas | ||
| using _ = await this.#mutex.acquire() | ||
| return await navigator.locks.request("AuthorizationCodeFlow.getCode", async _ => { | ||
| this.#authorizationUri = authorizationUri |
There was a problem hiding this comment.
Probably best to do this after the check of whether the signal is aborted.
2 participants
Best reviewed ignoring whitespace.