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.

Sign up for GitHub

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

Jump to bottom

Format functions may return objects for streams in objectMode #272

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

Format functions may return objects for streams in objectMode #272

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 6 additions & 1 deletion index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 •
edited

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.

stream.write(line)
}
else {
stream.write(line + '\n')
}
};

if (immediate) {
Expand Down
49 changes: 49 additions & 0 deletions test/morgan.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1075,6 +1075,55 @@ describe('morgan()', function () {
.get('/')
.expect(200, done)
})

it('should allow objects to pass-through for streams in object mode', function (done) {
var cb = after(2, function (err, res, line) {
if (err) return done(err)
assert.deepEqual(line, { method: 'GET', url: '/', statusCode: 200 })
done()
})

var stream = {
writableObjectMode: true,
write (obj) { cb(null, null, obj) }
}

function format (tokens, req, res) {
return {
method: req.method,
url: req.url,
statusCode: res.statusCode
}
}

request(createServer(format, { stream: stream }))
.get('/')
.expect(200, cb)
})

it('should convert objects to string for streams not in object mode', function (done) {
var cb = after(2, function (err, res, line) {
if (err) return done(err)
assert.strictEqual(line, '[object Object]')
done()
})

var stream = createLineStream(function (line) {
cb(null, null, line)
})

function format (tokens, req, res) {
return {
method: req.method,
url: req.url,
statusCode: res.statusCode
}
}

request(createServer(format, { stream: stream }))
.get('/')
.expect(200, cb)
})
})

describe('a string', function () {
Expand Down