Add new option bypassOnFinish, for when response is required in bypass callback

[ineentho]

In the current bypass implementation we only have access to the request object (because the response hasn't yet been built).

Sometimes it could be useful to have access to the res object too, if we for example would like to exclude all 404 responses.

bypassOnFinish: (req, res) => res.statusCode === 404,

This, in my opinion, contributes a better fix to the concerns raised in #90. In a more complex app with 100s of routes, it's not maintainable to manually add the middleware to each endpoint.

It may look slightly sketchy to run startTimer() without completing it, but it's ok because all startTimer does is save the current timestamp and return a callback, so the pending timer will simply be garbage collected.

I'm open to suggestions on the naming, but I couldn't come up with anything better.

Thanks for your work on this project!

[disjunction]

Thanks for contributing. I agree with your reasoning about standalone startTimer not posing any trouble. So given this fact, what prevents us from adding a second option param to bypass callback a second param res and moving the bypass to onFinished(...) for good?

[ineentho]

I thought this was the safer option because

• We don't know what the user has done to the req object in middlewares and request handlers, so existing checks could change
• Performance? bypass avoids adding the on finish listener, but this is probably very minor

Thinking about it a bit more, it's probably very unikely that we break existing code, so it makes sense to move bypass to onFinished. If you think that's the correct forward, I'll gladly update the PR.

[ineentho]

After further testing, I think moving bypass to onFinished definitely would be a breaking change; req.url is changed by express as the request is processed, see http://expressjs.com/en/api.html#:~:text=and%20so%20on.-,req.originalUrl,-req.url%20is. This even breaks our own test.

One option, if we want to keep the api nicer, is to check if the user passes a second parameter to bypass (opts.bypass.length === 2), and only in that case call it in onFinished. Not sure if that's a good idea though, I would probably concider that as too much unintuitive "magic".

[disjunction]

@ineentho what about bypass: (req: Request, options?: { checkOnFinish?: boolean, res?: Response }) => boolean ?

[disjunction]

... alternatively:

(req: Request | Partial<{ checkOnFinish: boolean, req: Request, res: Response }>) => boolean

[ineentho]

Sorry, not sure if I get what you mean exactly. Do you mean something like this?

bypass?: ((req: Request) => boolean) | { checkOnFinish?: boolean, callback: (req: Request, res: Response) => boolean }

That could work if you want to keep the number of configuration options down, sure.

[disjunction]

my bad - writing nonsense, but somehow you understood me. I meant making it accept optionally an object and this way reduce the number of options. It's already too high for this dumb library.

Next iteration:

bypass?: (
  (req: Request) => boolean) |
  {
     onRequest?: (req: Request) => boolean,
     onFinish?: (req: Request, res: Response) => boolean,
  }
)

sorry for the ping-pong. And lmk if you want to proceed or i do it myself. Yet I only have time on the weekend to be honest.

[ineentho]

That's a good idea, I think that's the best we are going to come up with :) I made a new commit with the changes, feel free to let me know if I should do anything differently.

[disjunction]

again, thanks for contributing. Released on npm as version 6.6.0