Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set function name to newFn #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

captDaylight
Copy link

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

Ref #16

@davidbanham
Copy link
Owner

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
Copy link
Collaborator

@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
Copy link
Owner

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

@vladimyr
Copy link

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 ⚠️
Can we wait until @kevinburkenotion responds to #16 ?

Regarding major/minor issue, FWIW I'm also in minor camp 👍

@davidbanham
Copy link
Owner

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

@Alexsey
Copy link

Alexsey commented Jul 3, 2019

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

@davidbanham
Copy link
Owner

davidbanham commented Aug 8, 2019

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
Copy link

Alexsey commented Oct 13, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants