[Fixes #163] Don't propose "default" format when provided with an Object as fmt

[javierfernandes]

Changing default usage of the "default" format, which is deprecated to suggest using the "combined" format.

This fixes this case morgan({})
It was proposing you to use morgan('default') which is deprecated. Now it says

morgan deprecated morgan(options): use morgan(format, options) instead

Testing

I've added a new kind of test (/test/deprecations.js) because /test/morgan.js was suppressing all deprecation message.
There was no other test trying to capture and assert deprecations.

Also, I believe that this new kind of test could help this other PR #153 that was criticized because it was not asserting about the deprecation message.

Note

Notice also that this doesn't actually handle the problem of trying to use morgan in a wrong way, using it directly as a middleware instead of calling it as a function to get the middleware.
That can be done (with one of this new kind of test :P) introducing a new check here

  if (format && typeof format === 'object') {
    opts = format
    fmt = opts.format || 'combined'

    // smart deprecation message
    if (format.baseUrl) {
       deprecate('morgan(options): instead of "app.use(morgan)", you must use morgan like this "app.use(morgan(\'combined\')"')
    } else {
      deprecate('morgan(options): use morgan(' + (typeof fmt === 'string' ? JSON.stringify(fmt) : 'format') + ', options) instead')
  }

Note that format.baseUrl is basically just a way to try to recognize that the first arg is express "request" object, which means that you have used morgan fn directly as a middleware. Maybe there is a better way to check that :)

I'll leave that to you or another PR, since I'm not sure about the deprecation message.
And actually I'm not sure that case should be handle as a deprecation warning message. I think that in that case the whole thing should blow up with an exception, because it won't probably work at all. Right ??

Cheers !!

[javierfernandes]

I changed this one also thinking that if "default" is considered as deprecated then it shouldn't be used as a default, right ?

But, although I could create a unitary test for this function along, it didn't make much sense. Then I was not able to create a particular test for this change. I just make sure all other tests kept running fine.

But it would be great to get a confirmation about this change.
Thanks !

[dougwilson]

Yea, this change will break people, so you'll have to determine a different way to make the change. The formats are slightly different, which is why one is deprecated and didn't just change the default without a message if that makes sense.

[dougwilson]

So this will break existing logs due to the format change, right? The date formats are different between default and combined, so you can't just change the default value of format.

[dougwilson]

And actually I'm not sure that case should be handle as a deprecation warning message. I think that in that case the whole thing should blow up with an exception, because it won't probably work at all. Right ??

Yes, though this is a fundamental issue you can do to any middleware module out there. Perhaps there is a change you can make to Express on this to catch the mistake instead of needing to change every single middleware that exists with whatever the check they should use is. That would be the biggest win for everyone 👍

[dougwilson]

Rather than disabling it, I would follow the suggestion of the linter and use safe-buffer instead.

[javierfernandes]

Ok, I thought you wouldn't like to add a new package just for this.

[dougwilson]

It's just a devDependency since this is a test.

[dougwilson]

Personally, I don't bother writing tests for these deprecation messages at all, but if you feel so inclined, I would say please don't do it by replacing process.stderr.write -- mocha itself may try to write there. Instead capture the messages through a listener: https://www.npmjs.com/package/depd#processondeprecation-fn

[javierfernandes]

I took it from https://github.com/dougwilson/nodejs-depd/blob/master/test/support/capture-stderr.js

Maybe you should also fix that :)

[dougwilson]

No, because that is the module itself, to validate its writing to stderr. Modules embedding depd should use the listener hook only (i.e. they should follow the README).

[dougwilson]

I think there is a typo somewhere as the description doesn't match the actual test. From looking at it, I think the description is what just needs the update 👍

[dougwilson]

Nice! This makes a lot of sense -- it's basically just telling you to use (format, options) instead of echoing a format you didn't provide. That makes a lot of sense.

[javierfernandes]

(couldn't reply to the comment inlined)

Yes, though this is a fundamental issue you can do to any middleware module out there. Perhaps there is a change you can make to Express on this to catch the mistake instead of needing to change every single middleware that exists with whatever the check they should use is. That would be the biggest win for everyone

That is not possible unless you are in a typed language. Express app.use() expects a function, and morgan is a function as well as the return value from morgan().
There is no way express can check that. Not even checking cardinality because I that would restrict you from doing something like app.use( (...args) => )

[dougwilson]

Then if it's not possible to fix, then I think it's best left to the README to show the proper usage. Like I said this issue can happen to any middleware, and if the solution is to go and pull request every single middleware out there, then I am a big 👎 on that and won't take a change in this module for that. If the solution is to used a typed langauge, then you can use typescript which would have caught this.