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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added enableObjectStream option #194

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

SnakeA
Copy link

@SnakeA SnakeA commented May 7, 2019

Background

I've got a similar use case with the issue raised: #181.

I am using Morgan for a lot of my projects and I would like to be able to write an Object in a stream instead of just a string.

Reason is that I am using Winston which supports meta-data for reach-er logs. I would like to use morgan and pass an object to Winston - instead of parsing a string.

Changes

  • Added a backwards compatible enableObjectStream in morgan() call.
  • Defaults to false
  • It needs a custom format function and a defined stream to work.
  • The only diff is that if the above are true (customFormatFunction && stream && enableObjectStream), we are not concatenating a \n (newline char), hence morgan can stream an object.

Any improvements/changes are welcome.

TODO:

  • Update README
  • Update HISTORY

but I'd like to get some opinions first 馃槃

@SnakeA
Copy link
Author

SnakeA commented May 10, 2019

@dougwilson Could you please have a look 馃槃 Thanks !

@dougwilson
Copy link
Contributor

I will after the express 4.17 release is out, as there was a premature vulnerability listed and it is our number 1 priority right now, I hope you understand.

@SnakeA
Copy link
Author

SnakeA commented May 10, 2019

@dougwilson
Thank you for getting back to me 馃槃 No worries I do understand that you have other priorities

I did some digging and I've realised that a lot of people tried to make this change to Morgan but it never went out.

Morgan is being used by numerous of our project and does the job, other than the fact that it emits a string that we have to parse.

It would be great if we can work something out, otherwise, we might have to implement our own middleware

Thanks again and looking forward to your review & opinion

@SnakeA
Copy link
Author

SnakeA commented May 23, 2019

@dougwilson

Any updates?
I can see that express 4.17 was released last week?

Thanks!

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

Sorry, still getting reports about 4.17 from users. Here is a very quick review for points to work on:

  1. No docs for new feature in pr
  2. Does not work right when buffer feature enabled
  3. Silently does nothing when using built in formatters. It could throw an error or at least be documented.

I hope this feedback helps while I am working on 4.17 issues in parallel.

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.

None yet

3 participants