Skip to content

feat: support promise-returning tokens - #380

Open
ahmed-sherif-hassona wants to merge 4 commits into
expressjs:masterfrom
ahmed-sherif-hassona:feat/async-tokens
Open

feat: support promise-returning tokens#380
ahmed-sherif-hassona wants to merge 4 commits into
expressjs:masterfrom
ahmed-sherif-hassona:feat/async-tokens

Conversation

@ahmed-sherif-hassona

Copy link
Copy Markdown

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

  • Support Promise/thenable values returned by tokens.
  • Preserve existing synchronous token behavior.
  • Add regression tests for async tokens.

Testing

All existing tests pass, along with the new async token tests.

@kilisamemarisaaa kilisamemarisaaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread index.js Outdated
// 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) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Comment thread index.js Outdated
return
}
debug('log request')
stream.write(str + '\n')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread index.js
}
debug('log request')
writeLog(str)
}).catch(function (err) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread index.js Outdated
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') {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 kilisamemarisaaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 passing
  • npm run lint: passed
  • git 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants