Package will throw under Express v5

[jonchurch]

Hey there! Wanted to give you some heads up that express v5 should be getting released sometime ~next month.~

Version 5 will support returning promises from route handlers. But there have also been changes specifically to Router which will make this package throw a fatal error currently. Both Layer and Router are no longer able to be deep required through require('express/lib/router). Router has been moved into its own package at pillarjs/router and is used as a dep of express.

I'd suggest maybe checking the version of Express that is being used when users require this module and printing a deprecation notice or something similar to avoid crashes.

const {version: expressVersion} = require('express/package.json');

if (expressVersion[0] > 4) {
    console.log(`deprecated: express-async-errors works with version 4 of Express, you are using ${expressVersion} which supports async route handlers natively`)
} else {
    // setup module
}

I've used your package a bunch in the past, so thank you for creating it ❤️

If there's anything I can do to help or answer any questions about v5, please let me know.

To test out the latest release candidates of Express before v5 is released into main, you can npm i express@next.

P.S. I see that you've also promisified Router.param which I'm not 100% sure is coming to Express v5. I'll have to check on that.

[davidbanham]

Oh hey thanks for the heads up! Much appreciated.

That's super neat that this is getting baked directly into Express. Is the async route handling in Express 5 similar enough to what this package provides that we can just print the deprecation notice and return or should we be printing the deprecation and then throwing to ensure that users aren't expecting/relying on behaviour they'll no longer be getting?

Is there anything clever that we could be doing with npm peer dependencies to ensure this never gets installed alongside express v5 or above?

[jonchurch]

Here's the PR with just the changes to handling promises from route handlers, and here's the PR with the new version of router.

It's essentially the same behavior, but a different implementation. If a promise is returned from a middleware, add a handler to call next on a rejection with the error.

A difference though is that v5 doesn't have plans to return promises from Router.param functions.

I'm not sure how it could be solved through peer deps. Maybe there is a way but I'm not very familiar with their usage, I know they are more of a warning than a restriction and I think they aim mostly to help a package install a version of a dep they rely on (which wouldn't be desired behavior in this instance).

The scenario I envision is folks updating their existing Express apps in development during migration from 4 to 5 without a clean reinstall of all their deps. I don't think peer deps would help in that situation.

[davidbanham]

Okay cool, let's abandon the idea of peer deps then.

I think the only remaining decision here is whether we should:

console.log(deprecationNoticeMessage);
return;

or

console.log(deprecationNoticeMessage);
throw new Error(deprecationNoticeMessage);

I think the answer to this lies in those two specific PRs but unfortunately I'm just too far out of the Express world to really grok them and make a call on that. Can anyone else chime in on this?

[jonchurch]

It's up to you and the experience you want users to have, throwing would be fast fail and crash the app intentionally. If throwing, the dep notice should prompt people to uninstall this package in order to get their app running again.

Returning would let the app run without removing the package.

Whatever your choice, we should likely note in the Express migration guide from v4 to v5 that folks can uninstall packages they were using to catch rejected promises returned from middleware/handlers.

I mentioned the feature you add with this package which Express 5 doesn't currently plan to implement, making router.param compatible with promises ala https://github.com/davidbanham/express-async-errors/pull/12. I see an option for you to preserve that functionality for v5 users, but to not touch the route handler logic. That would allow folks who rely on this package to keep the same features when upgrading to v5.

I'm happy to answer any questions or make a PR implementing the above.

Btw we discussed the router.param feature you've implemented in the Express TC Meeting today at the end of the meeting. No decisions made, but it could get added. You're welcome to make a PR against the router repo. I'm not sure when it would land, but I think it could make it into a minor of v5 if folks wanted it.

[davidbanham]

If you've got a vision to a way to preserve the goodies from #12 without interfering with the new v5 router then a PR would be very gratefully received, thanks!

Unfortunately I don't think I'm the person for the job of making a PR against the pillarjs repo. I'm not actually writing much node these days as I've wandered off into other languages. As such I just don't have the currency of knowledge and context to be an effective contributor to core infra like that.

[jonchurch]

Understood 👍

I've opened #29 as a draft PR for hashing out and tracking the changes that would need to be made. I plan to involve more folks from Express so we can figure out what needs to be done.

Thank you for being responsive to changes and comments, especially in these crazy times ❤️

[dougwilson]

Hey @jonchurch would you be willing to make a pull request to the router repo to add Promise support to router.param ?

[jonchurch]

I've opened a PR https://github.com/pillarjs/router/pull/92 to add promise support to router.param which looks like it'll be making it into v5 of Express. 🎉

[jonchurch]

@davidbanham been a while! emailing you about possibly xferring the package into express <3