Skip to content
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

Format functions may return objects for streams in objectMode #272

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gfoust
Copy link

@gfoust gfoust commented Dec 17, 2022

Currently Morgan converts the return value of a format function to string (by means of concatenating it with a new-line). In some cases it would be convenient to allow objects returned by the format function to be passed directly to the stream, provided that the underlying stream can consume objects.

A stream that operates in "object mode" can operate on objects other than just strings and buffers. (See https://nodejs.org/docs/latest-v19.x/api/stream.html#object-mode) The writableObjectMode flag of a stream can be used to determine whether a stream is operating in object mode. (See https://nodejs.org/docs/latest-v19.x/api/stream.html#writablewritableobjectmode) This has been part of the Node API since version 12.

This change simply alters Morgan so that it does not concatenate the return value of a format function with a new-line as long as (1) the return value is an object, and (2) the underlying stream's writableObjectMode flag is truthy. It also adds two unit tests: the first verifies that an object will be passed to the stream without modification if it is in object mode; the second verifies that an object will be converted to a string if the stream is not in object mode (as is the current behavior). I did not make any changes to the README, but I would be willing to if you were intending to accept this request.

As an example of why this is useful, the following code snippet shows how this change allows Morgan to work in conjunction with Winston to create an access log in JSON format. By providing the data as an object instead of a string, the individual pieces of data can be easily utilized by the underlying logger.

app.use(morgan(
  function (tokens, req, res) {
    return {
      method: tokens.method(req, res),
      url: tokens.url(req, res),
      status: Number.parseFloat(tokens.status(req, res)),
    }
  },
  {
    stream: {
      writableObjectMode: true,
      write(message: any) { logger.log({ level: 'http', ...message }) }
    }
  }
))

This change does alter existing behavior. However, the objectMode option for streams is false by default. (See https://nodejs.org/docs/latest-v19.x/api/stream.html#stream_new_stream_writable_options) More specifically, it is false in process.stdout, in streams created by fs.createWriteStream, and in streams created by require('rotating-file-stream').createStream -- which are the three types of streams mentioned in the README. Furthermore, using built-in formats and format strings always results in a string value. Therefore, the only way this could be a breaking change is if a user (1) used a custom format function that returned an object, (2) used an underlying stream that had writableObjectMode set to true, and (3) was relying on Morgan to convert the object to a string. This seems highly unlikely, and, even if it did happen, could be fixed by merely updating the format function to convert its return value to string.

Note that there is currently a work-around for this issue, which is to make the format function return JSON, and the underlying stream parse the JSON, as described in https://betterstack.com/community/guides/logging/how-to-install-setup-and-use-winston-and-morgan-to-log-node-js-applications/#logging-in-an-express-application-using-winston-and-morgan. This works, but is inefficient and inelegant.

@agnjunio
Copy link

+1 Really need that feature for my project

@@ -127,7 +127,12 @@ function morgan (format, options) {
}

debug('log request')
stream.write(line + '\n')
if (stream.writableObjectMode && typeof line == 'object') {
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you made this a while ago, but I was just pointed to it, haha.

So I love this idea! I'm curious if there is the typeof line == 'object' is really needed at all or not -- for example if the line is a string, perhaps just give that as-is as well to the object-mode stream without adding the trailing newline. I could see it being useful for the stream to concatenate with a different character, for example.

Copy link
Author

@gfoust gfoust Feb 17, 2023

Choose a reason for hiding this comment

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

The only reason I added typeof line == 'object' was to minimize the changes to existing behavior. As the library stands right now, if you were to log a string, it would add a newline to that string regardless of whether your stream supported object mode or not. That type check was to make sure this patch would not alter that behavior.

I like your design (without the type check) better as a design, but I thought the patch would be more likely to be accepted if it did not change any existing behavior changed existing behavior as little as possible.

@jdelgadofrts
Copy link

Do we have any progress on accepting this change? It's a great addition for morgan+winston setups specially when the output is being consumed by third party services.

@tomer555
Copy link

+1

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

Successfully merging this pull request may close these issues.

5 participants