move formats one level under the morgan object

[ryhinchey]

This PR would close #190.

Per my comment #190 (comment), there is not an issue with using import syntax in the versions of node that support it. The issue is only with using import syntax via the esm module.

If we'd rather not land something like this under 1.x I'm fine with that call and am all for starting the work on v2 that removes the default format (along with other items) as noted here #222.

[dougwilson]

I can certainly say that it is not possibe actually land this on 1.x because it moves publically accessible API; the format function are now no longer available within the other tokens as the same name, for example.

I'm just noting that if that affects your desire for this PR, as it would be a v2 PR as-is, and it sounded like you had a different change in mind for v2.

[ryhinchey]

I can certainly say that it is not possibe actually land this on 1.x because it moves publically accessible API; the format function are now no longer available within the other tokens as the same name, for example.

I viewed this as an internal change that didn't effect the public facing api (what's available via module.exports). If someone is using morgan per the documented api, this shouldn't be a breaking change, right?

Going forward, it's good to understand what constitutes a breaking change. You're saying that because someone can access these directly on the morgan object and with this PR they'd no longer be able to, that it's a breaking change?

If that's the case (and I'm fine with it), I'll close this PR and make a PR adding tests that would prevent this PR from being made in the first place.

[dougwilson]

I viewed this as an internal change that didn't effect the public facing api (what's available via module.exports). If someone is using morgan per the documented api, this shouldn't be a breaking change, right?

What is on the module.exports is what is passed into your custom format function. This is documented public API, but docs can always be improved if needed. Here is an example app that would be broken with the PR here, which isn't doing anything non-public IMO:

var express = require('express')
var morgan = require('morgan')

var app = express()

app.use(morgan(function (tokens, req, res) {
  return someCondition() ? tokens.default(tokens, req, res) : tokens.dev(tokens, req, res)
}))

app.get('/', function (req, res) {
  res.send('hello, world!')
})

Going forward, it's good to understand what constitutes a breaking change. You're saying that because someone can access these directly on the morgan object and with this PR they'd no longer be able to, that it's a breaking change?

The strict definition is just that given by semver (https://semver.org/) of course. In this particular case, in the context of Node.js, what a module is adding onto the exports of it's module is, by definition, public API. This PR is directly altering the names of exports of the module, which seems to be a pretty straight-forward violation.

When I took over this module, it had almost no documentation, like many of the expressjs middlewares. I certainly tried to document as much as I could, but there may be gaps, and we can work to fill those. The same for the tests, they had little to no tests and I worked a lot to try and add as many tests as I could think of. Getting to 100% code coverage at least, though of course everyone knows 100% code coverage doesn't mean every use is actually covered, just the minimum base line to start at, so that is another area we can continue to improve if that helps too.

[ryhinchey]

This PR is directly altering the names of exports of the module, which seems to be a pretty straight-forward violation.

I took a look at module.exports and since I didn't change anything there, I figured this wouldn't be a breaking change. You're right though - given I'm changing the properties on the morgan object, this is indeed a breaking change.

Thanks for giving a thoughtful review here. It's very helpful!

I'm going to close this PR and submit a new one that adds tests that would have not passed for this PR.