morgan
morgan copied to clipboard
expressjs
Refactored deprecation message for `default` format
Ah, sorry I missed that...does my latest commit work? It's not checking for the case where no format is passed, but that already has its own deprecation message (undefined format: specify a format), so I figured that was fine.
Thank you, sorry I didn't see your previous message. It looks like there is no warning displayed on the usage of the deprecated property though? For example the following should produce a deprecated warning so folks know it will stop working in the next release:
app.use(morgan(`ACCESS ${morgan.default}`))
It shows the deprecation warning if you do: morgan('default'), which I thought was the standard way of doing it.
If morgan.default still needs to work, then unfortunately that defeats the whole purpose of this PR. esm assumes that module.exports.default is the default export of an ES6 module, which is why it's accessing it as soon as you import the module. The only way to solve it as far as I can tell is to ensure that module.exports.default no longer points to the default format function.
So the idea is to remove the module.exports.default property, so the deprecation warning is to let folks know to not use it since it's going to be removed.
Are you saying we shouldn't remove that property?
Actually I just thought of a solution that I think preserves the current behavior...in my initial testing it seems to work fine. It's a change to the exports at the top of the file:
Object.defineProperty(module.exports, '__esModule', { value: true })
module.exports.default = morgan
module.exports.compile = compile
module.exports.format = format
module.exports.token = token
morgan.compile = compile
morgan.format = format
morgan.token = token
Can you think of any compatibility issues with this approach? If not, I can do some more testing and update the PR.
I'm not familiar with that property, so no idea what it's effects would be.
We then see that we're setting the __esModule property of exports to true. This simply tells any system that imports this file that it just imported a module. If this option is off, some module resolution systems will assume that this file would put an object in the global scope and will execute it without trying to get any of its exports directly.
(from https://www.codementor.io/elliotaplant/understanding-javascript-module-resolution-systems-with-dinosaurs-il2oqro6e)
I'm guessing that as long as this solution works in native node.js and also with bundlers like webpack and rollup, that it should be fine? If all of those work, I can't think of a scenario where it would fail, unless older versions of node use a very different module resolution algorithm. I can test it with node 6 just in case.
P.S. The __esModule property is automatically set to true by Babel and other tools when they're transpiling ES6 modules to CommonJS modules. So if you intend to use import/export syntax in the source code for any future version of this module, __esModule will be set to true anyhow.
So from reading through Babel issues, etc. the __esModule === true is when the module is a ECMAScript module and not a CommonJS module. This is a CommonJS module, though. I'm trying to look at what issues this could case, but just at a high level, setting the __esModule to true doesn't seem correct, as this is, in fact, a Common JS module.
From my limited searching it would seem that setting __esModule to true, as your proposal is, would make folks who are using import morgan from "morgan" end up with morgan being a string and not the expected middleware function.
This is because when __esModule is not set, the Babel load will make module.exports the default export (which is how Common JS modules work), but when it is true, then the property with the name of default is treated as the default export.
After further investigation, I found that my proposed solution doesn't work the way I thought it would. (I think the only way that would work correctly is if it were a module imported by another module that had been transpiled from ES6, but obviously node users who aren't using any module bundler need to be able to require morgan directly.)
So this brings it back to a choice: either don't address this issue at all, or accept the fact that the deprecation warning would only show up with standard usage, i.e. morgan('default'). Any version of the code that keeps morgan.default pointing to the format function would not prevent the erroneous warning when using the esm module. In other words, if morgan(morgan.default) absolutely still needs to work as it did in previous versions and also show the deprecation warning, then the ESM issue can't be fixed.
The good news is that this library seems to work fine as-is with native modules support using node --experimental-modules; the issue only happens specifically with the 3rd party esm module. Since esm is a stopgap solution that will eventually become irrelevant once node finalizes native modules support, it would be understandable if you choose not to merge this PR.
And just to clarify here: we are sure this is not just some kind of esm bug?
Originally I didn't think it was an esm bug, but after looking into this I'm wondering if maybe it is...maybe it wouldn't happen if esm checked for that __esModule flag. It seems like it's treating morgan as an ES module even though it's not.
Looks like someone reported this same issue in the esm repo: https://github.com/standard-things/esm/issues/812
Thank you for the research you have put into this, it is really appreciated. Perhaps the best solution overall here is just to set some time aside this weekend and I will push out a 2.0 of morgan which will drop the .default export finally, which I think may be the best all-around solution. I'm not saying to close this PR per se; let me see what happens this weekend re: 2.0.
@dougwilson any updates on finally removing default key after 5 years that key 😭
