fix: restore 'debug' logs

[dpopp07]

Revert "Remove debug dependency", with the following changes:

• Update "var" to "const" for debug imports
• Preserve history file so intermediate versions are documented correctly
• Use latest version of debug

This reverts commit 0b5a0a6.

[bjohansebas]

LGTM, please add it to the history

[dpopp07]

Note: as-is, this does NOT restore the ability to use DEBUG=express:* for viewing router logs. I don't think we should lose the scope of this package, since it's decoupled from Express.

If it was really important to restore that behavior, we could do something hacky like:

const debugCurrent = require('debug')('router')
const debugLegacy = require('debug')('express:router')

function debug(...args) {
  debugCurrent(...args)
  debugLegacy(...args)
}

I tested this and it seems to work fine. That said, I'd prefer not to add something like this.

I would rather open a follow-on PR in the v5 migration guide to explicitly state that router logs must now be viewed through the router scope (DEBUG=express:* -> DEBUG=express:*,router).

Let me know what y'all think. I'm open to other options as well.

[dpopp07]

please add it to the history

Done! I thought that might need to wait until the release, forgot about the "unreleased" pattern. Thanks for pointing it out.

[dpopp07]

Re: my prior comment

I realized that if it's important to restore express:router debug logs and if we want to move in the direction of diagnostic channels, we could go ahead and implement diagnostic channels in this library that simply duplicate the current debug logs.

In Express, we could subscribe to this channel, re-print any messages with debug under the express:router namespace, and it should be just like old times. There may be hidden complexity and I haven't tested it but I think it should work.

@jonchurch this might have been what you suggested the other day, unless you were just proposing that diagnostic channels would be a better approach for the future. Either way, I'm curious what folks think about this.

[bjohansebas]

I think diagnostic channels would be for a different PR, and technically this is a separate package, so it doesn’t make sense for debug to be used for express:router. This is not a blocking comment, though, in case another member suggests that it should observe express:router.

[dpopp07]

Agreed, the express:router namespace should certainly not be defined in this package. The proposal is that an express:router debug namespace could be defined, in the express package, that effectively wraps messages published by the router package through a diagnostic channel.

I only suggest this as an option, in the case that it is important to restore express:router debug logs in the express package to what they were in v4 before going latest with v5. If it's not, I would prefer merging this as-is and updating the migration guide accordingly.

Update: My original motivation for suggesting this is that the related issue referred to expected logs missing from express:*. This comment says:

An aside, splitting like this makes it more difficult to debug router under the express:* namespace. Not impossible, but something to solve for.

This is a potential, though not ideal, solution.

[bjohansebas]

At this point, the best option is to update the migration guide about the fact that express:router in debug no longer exists, since it was a breaking change and no longer exists in [email protected].

The diagnostic channel would be great to try in the future.

[dpopp07]

I agree. Unless there are any objections before I get to it, I'll go ahead and open that PR against the migration guide

[dpopp07]

I opened the follow up PR to the migration guide: expressjs/expressjs.com#1819

This PR will need to be merged and released before that documentation is accurate.

[wesleytodd]

Awesome! I don't think we should bring back express:* to include router, but it might mean we need to add some migration docs on this right? This is technically a breaking change we did already by removing it, so adding it here as the router scope is pretty reasonable and forward moving imo.

[dpopp07]

Sounds good!

it might mean we need to add some migration docs on this right?

Yes - I opened a draft PR updating the migration docs. See my previous comment 🙂