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

changed default stream to <<stdout+'\n'>> #17

Closed
wants to merge 4 commits into from

Conversation

amitport
Copy link

@amitport amitport commented Jun 4, 2014

changed to code so that the default stream is stdout with a line break at the end.
Before this change the default stream was stdout and the line break was hard-coded during the write operation.

This prevented using a custom streams which do not require or expect line breaks (e.g., winston transports)

changed to code so that the default stream is stdout with a line break at the end. 
Before this change the default stream was stdout and the line break was _hard coded_ during the write operation. 

This prevented using a custom streams which do not require or expect line breaks (e.g., winston transports)
@dougwilson
Copy link
Contributor

Thanks! This is a good idea, but it is super breaking and will just have to wait for the next major. People are already passing in their own custom streams and expecting the newline to be at the end, which this removes.

@dougwilson dougwilson self-assigned this Jun 4, 2014
@amitport
Copy link
Author

amitport commented Jun 4, 2014

if this is too much of a breaking change I have another suggestion:

  1. add options.write- a function that receives a formatted log line and takes care of writing as it see fit
  2. deprecate options.stream
  3. deprecate options.buffer which is a concern of the log writing aspect, and should be handled by a specialized library (e.g., winston)
  4. changed the code so that options.write will take precedence over options.stream when options.write is defined

@dougwilson
Copy link
Contributor

deprecate options.buffer which is a concern of the log writing aspect, and should be handled by a specialized library (e.g., winston)

I agree and was already planning that :)

add options.write- a function that receives a formatted log line and takes care of writing as it see fit. deprecate options.stream

i think it should still be options.stream--stream is a universal interface in node.js and if you call it write, people will want to do {write: mystream.write} which would fail, since the context of the write function is lost; they have to remember to bind first, which is weird.

@amitport
Copy link
Author

amitport commented Jun 4, 2014

Having a direct write option is not very important, though I think is is reasonable to support both a direct function and an object by doing something like:

if (typeof options.stream.write !== 'function')
  options.stream  = {write: options.stream};

I any case, I think it is important to rename options.stream, deprecate the old one, and change the documentation. It will just to help people make a smoother transition.

how about options.out for a new name?

@dougwilson
Copy link
Contributor

So after thinking on it, I'm going to close this. The reason is that this module creates text lines and streams them as a text stream. It sounds like you just want a stream of lines, rather than a stream of text. If you want your stream to be split on lines, simply pass in a stream that uses something like https://www.npmjs.org/package/split

@dougwilson dougwilson closed this Jun 11, 2014
@dougwilson
Copy link
Contributor

Example:

var morgan = require('morgan')
var split = require('split')

var myStream = ... // your stream you want to get line events to

app.use(morgan({
  stream: split().pipe(myStream)
}))

@amitport
Copy link
Author

FWIW I don't agree - the are use cases both ways
In my case, I'm using winston, and it just leaves me with a lot of boilerplate...

var morgan = require('morgan')
var split = require('split')
var winston = require("winston");
var winstonStream = require("winston-stream");
var log = winstonStream(winston, "info");

app.use(morgan({
  stream: split().pipe(log)
}))

@dougwilson
Copy link
Contributor

OK, but the reverse is true for all the people who log to a file stream, then. We want no boilerplate for this pattern:

var fs= require('fs')
var morgan = require('morgan')

app.use(morgan({
  stream: fs.createWriteStream('access.log', {flags: 'a'})
}))

@dougwilson
Copy link
Contributor

Also, your example it not useful, because the winston-stream library is stripping the newline we write, so I don't see the problem; you don't even need to use split with the winston-stream library...

var morgan = require('morgan')
var winston = require("winston");
var winstonStream = require("winston-stream");
var log = winstonStream(winston, "info");

app.use(morgan({
  stream: log
}))

@expressjs expressjs locked and limited conversation to collaborators Jul 17, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants