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

API wart: server.dependency after method lacks options #2520

Closed
garthk opened this issue May 1, 2015 · 4 comments
Closed

API wart: server.dependency after method lacks options #2520

garthk opened this issue May 1, 2015 · 4 comments
Assignees
Labels
Milestone

Comments

@garthk
Copy link

@garthk garthk commented May 1, 2015

I find myself having to do this a fair bit:

function register(server, options, next) {
    server.dependency(['auth-plugin'], function (_server, _next) {
        after(_server, options, _next));
    }
}

function after(server, options, next) {
    // ... register routes, etc
}

Sure, I could use closures… but if the after API is designed around closures, why are we passing it server? If there's a reason to pass server, there's a reason to pass options.

Do we have a bug category for "API changes waiting for enough friends to justify a major version bump"?

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented May 21, 2015

What's the issue against using a closure here? I understand the question but I'm trying to think if passing server is that big of a deal to bother with.

@garthk
Copy link
Author

@garthk garthk commented May 29, 2015

We fixed the 1-ary callback style for server methods because it stood out. This stands out in the same way, for me. We force use of closures to get options, but not to get server. Most of the API, if I ask "why?", I learn something useful from the answer. This, not so much. I think it's a wart.

To fix it, we could get away with calling after with just next and force use of closures for both server and options. I think it's tidier to echo the registration API and call with server, options, next, but I could argue for either way.

That said, I wouldn't bump the major for just this wart. I'd batch up a bunch of wart fixes.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Aug 11, 2015

I decided to keep the API as is but to expose the options via the realm object so you no longer need a closure.

@hueniverse hueniverse closed this Aug 11, 2015
@Marsup Marsup added feature and removed request labels Sep 20, 2019
@lock
Copy link

@lock lock bot commented Jan 9, 2020

This thread has been automatically locked due to inactivity. Please open a new issue for related bugs or questions following the new issue template instructions.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants