multer icon indicating copy to clipboard operation
multer copied to clipboard

fix: wrap middleware in promise

Open InsOpDe opened this issue 3 years ago • 4 comments

this is necessary, so that async hooks work

#814 #1046

InsOpDe avatar Jul 07 '22 07:07 InsOpDe

tests all pass.

no new tests have been added

InsOpDe avatar Jul 07 '22 07:07 InsOpDe

Thank you for the PR! Currently there are unfortunately two problems which means we cannot merge it yet:

  • This is a breaking change, since Multer 1.x supports Node.js 0.10 which doesn't have Promise
  • If next throws an error that will be caught by us, and then we will call next again with that error. This is a very common misstake to make when trying to "callbackify" a promise. You can look here for some info on doing it the "correct" way: https://github.com/LinusU/util-callbackify/blob/master/index.js

LinusU avatar Jul 07 '22 11:07 LinusU

Hi @LinusU do we have tests which check for your second point?

I wouldn't know how to fix the problem without promises. Should we close that PR then?

InsOpDe avatar Jul 07 '22 11:07 InsOpDe

[...] do we have tests which check for your second point?

Not at the moment, but this would be a great to add!

I wouldn't know how to fix the problem without promises. Should we close that PR then?

One idea could be to only enable this path if typeof Promise === 'function'. I'm not to familiar with the context but I think that this can be solved without wrapping the entire thing in a Promise, instead using the async_hook module to track the context.

There seems to be some documentation here: https://nodejs.org/api/async_context.html

This could then be imported something like:

var asyncHook
try {
  asyncHook = require('async_hook')
} catch (_) {}

LinusU avatar Jul 07 '22 11:07 LinusU