feat: support promise-returning tokens - #380
Conversation
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Verified on head 9f77580 with Node 24.12.0. The existing 96+6 tests and lint pass, but the two async failure and compatibility paths below are uncovered and reproducible locally. I cannot mark request-changes without repository access, but both findings should be addressed before merge.
| // Support Promise-returning format functions and compiled formats with | ||
| // async tokens: if formatLine returned a thenable, defer the write. | ||
| if (line && typeof line.then === 'function') { | ||
| line.then(function (str) { |
There was a problem hiding this comment.
[P1] Handle rejected async tokens before returning. This attaches only a fulfillment handler, and logRequest is invoked from the response-finished event so no caller can await or catch the returned Promise. A token that returns Promise.reject(new Error("token failed")) therefore becomes an unhandled rejection; with Node 24 defaults, a one-request server exits with status 1 and the stack points through the generated format into this branch. Please apply an explicit rejection policy here so token failures cannot terminate the host process, and add a rejecting-token regression test.
| return | ||
| } | ||
| debug('log request') | ||
| stream.write(str + '\n') |
There was a problem hiding this comment.
[P2] Route resolved values through the existing object-mode write path. The synchronous branch below passes an object returned by a format function directly to a writableObjectMode stream, but Promise.resolve({ kind: "async" }) reaches this line and is written as the string "[object Object]\n". I reproduced the two formats against the same object-mode Writable: sync produced an object while async produced a string. Promise-returning format functions should preserve the same output contract as synchronous formats; a shared write helper plus an object-mode regression test would cover both branches.
There was a problem hiding this comment.
Thanks for the review! I’ve addressed both the P1 and P2 issues:
Added handling for rejected async tokens.
Preserved object-mode behavior for async format results.
Added regression tests for both cases.
All tests are passing.
| } | ||
| debug('log request') | ||
| writeLog(str) | ||
| }).catch(function (err) { |
There was a problem hiding this comment.
[P1] Assimilate thenables before attaching catch(). A Promise/A+ thenable is allowed to return undefined from then; for example, { then (resolve) { resolve('ok') } } writes ok\n and then throws synchronously here because the return value of line.then(...) is not chainable. I reproduced this on Node 24.12.0 against head 9363bb3, and the exception escapes the request handler. Wrapping first with Promise.resolve(line).then(...).catch(...) (or otherwise normalizing the thenable) preserves rejection handling for native Promises and prevents this valid thenable from crashing the host.
There was a problem hiding this comment.
Thanks for catching this. I’ve normalized the thenable with Promise.resolve() before attaching the handlers and added a regression test for a thenable whose then() returns undefined.
The test suite passes.
| stream.write(line + '\n') | ||
| // Support Promise-returning format functions and compiled formats with | ||
| // async tokens: if formatLine returned a thenable, defer the write. | ||
| if (line && typeof line.then === 'function') { |
There was a problem hiding this comment.
[P1] Guard access to then before applying the rejection policy. A thenable may expose then through a getter, and Promise resolution treats an exception from that getter as a rejected promise. Here the getter is read by typeof before Promise.resolve(line) runs, so a value such as Object.defineProperty({}, 'then', { get () { throw new Error('then getter failed') } }) escapes synchronously from the response-finished callback instead of reaching the new async-token error path; I reproduced this against the current head with Node 24 and observed uncaught: then getter failed. The generated checks from compile() have the same unguarded read. Please guard then-property lookup under the rejection handling and add regressions for both a promise-returning formatter and a compiled string token.
kilisamemarisaaa
left a comment
There was a problem hiding this comment.
Rechecked the throwing-then-getter fix on exact head 9295f5ae0da8d53dbaa4613905b22c53290ccb0e.
Both previously unguarded lookups now go through hasThenProperty; when the getter throws, the value is deliberately passed to Promise.resolve, which converts that access failure into the existing handled rejection path. The new regressions cover both a promise-returning formatter and a compiled string token.
Local verification on Windows / Node 24.12.0:
npm test: 101 + 6 passingnpm run lint: passedgit diff --check origin/master...HEAD: passed
My latest finding is resolved on this head. I did not find another blocking regression in the changed paths.
Description
This PR adds support for Promise-returning custom tokens in Morgan.
Previously, an async token could result in
[object Promise]being written to the log instead of the resolved value.Changes
Testing
All existing tests pass, along with the new async token tests.