search

davidbanham / express-async-errors

async/await support for ExpressJS
Other
900 stars 43 forks source link

Set function name to newFn #17

Open captDaylight opened 5 years ago

captDaylight commented 5 years ago

Simply sets the function name to the new function during wrap.

Ref #16

davidbanham commented 5 years ago

Thanks for the PR! This looks good to me.

@kronicker Can you think of any possible unintended consequences of this before we merge?

I think this is only a minor version bump, but can anyone think of a way it might break existing behaviour requiring a major?

kronicker commented 5 years ago

@davidbanham I don't think so. Express does not rely on function names anywhere, only on arity. Thinking about it, I think I wanted to do this originally. If we want to be nitpicky, wrapper function will now have the same name as the original wrapped one so they will appear with the same name in the stack, but I guess that's fine and still better then 'newFn'. The minor bump should be fine.

davidbanham commented 5 years ago

Awesome! Thanks for taking a look. I'll release tomorrow.

vladimyr commented 5 years ago

If we want to be nitpicky, wrapper function will now have the same name as the original wrapped one so they will appear with the same name in the stack, but I guess that's fine and still better then 'newFn'. The minor bump should be fine.

Let's be nitpicky cause this thing can easily confuse people and is pretty nonstandard :warning: Can we wait until @kevinburkenotion responds to https://github.com/davidbanham/express-async-errors/issues/16 ?

Regarding major/minor issue, FWIW I'm also in minor camp :+1:

davidbanham commented 5 years ago

Yep, happy to hold off on the release while we wait to hear back on #16. Thanks, @vladimyr

Alexsey commented 5 years ago

@vladimyr How about making it fn.name + 'Wrap' for not been confusing? 'Wrap' may be even something more specific, like 'throwHandlerWrap'

davidbanham commented 5 years ago

Seems like we're all happy to move ahead on the fn.name + 'Wrap' mechanism?

@vladimyr does that work for you?

If there's no howls of dissent in the next week or so I'll update this PR with the Wrap suffix on the name and look to release it.

Alexsey commented 5 years ago

@davidbanham are you going to change fn.name to fn.name ? fn.name + 'Wrap' : 'throwHandlerWrap'?