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

Replacing plugin.dependency() with attributes key #2332

Closed
osslate opened this issue Jan 4, 2015 · 11 comments
Closed

Replacing plugin.dependency() with attributes key #2332

osslate opened this issue Jan 4, 2015 · 11 comments
Assignees
Labels
Milestone

Comments

@osslate
Copy link

@osslate osslate commented Jan 4, 2015

Would it be a cleaner idea to replace plugin.dependency() with an extension to register.attributes?

Rather than

var resolved = function (server, next) {

    server.route({}) // etc
    next()
}

var register = function (server, options, next) {

    server.dependency("lout", resolved)
    next()
}

register.attributes = {
    name: "test",
    version: "1.0.0"
}

it'd be possible to do something like this

var register = function (server, options, next) {

    server.route({})
    next()
}

register.attributes = {
    name: "test",
    version: "1.0.0",
    dependencies: "lout"
}

I can't see why this wouldn't work out at first look.

@bendrucker
Copy link
Contributor

@bendrucker bendrucker commented Jan 4, 2015

At very least it's a breaking change. What if I don't know that I need a particular dependency until something async happens? At that point you need to maintain two interfaces and I think the documentation cost is too high.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 5, 2015

I don't mind it if others like the idea, but only as an additional option, not a replacement. There are plenty of use cases for dynamic dependencies, especially when dealing with test vs. production. I'll leave it open for a bit to see if others find it significantly cleaner to be worth the extra complexity.

Loading

@nlf
Copy link
Member

@nlf nlf commented Jan 5, 2015

I kind of like the idea, but I definitely agree this would need to be an "in addition to" and not a breaking change.

For some things this would be perfect, but like @hueniverse said there's some value in being able to dynamically declare your dependencies as well.

Loading

@kamilwaheed
Copy link

@kamilwaheed kamilwaheed commented Jan 7, 2015

+1 for adding the ability to mention dependencies under register.attributes; as an enhancement, of course.

Loading

@devinivy
Copy link
Member

@devinivy devinivy commented Jan 14, 2015

I think this syntax for including dependencies, all so familiar to node developers, would be a nice feature addition. Is the extra complexity high?

Loading

@osslate
Copy link
Author

@osslate osslate commented Jan 14, 2015

I can't imagine the complexity of implementing is too bad, can send a PR with this implemented since it seems to be getting positive feedback. Should it be register.attributes.dependencies or register.dependencies?

Loading

@nlf
Copy link
Member

@nlf nlf commented Jan 14, 2015

I think register.attributes.dependencies IMO

Loading

@devinivy
Copy link
Member

@devinivy devinivy commented Jan 14, 2015

I'd agree.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Jan 17, 2015

I'll consider a PR.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 9, 2015

If anyone still cares about it, just open a pull request implementing the additional option.

Loading

@hueniverse hueniverse closed this Feb 9, 2015
@hueniverse hueniverse self-assigned this Feb 9, 2015
Marsup added a commit to Marsup/hapi that referenced this issue Feb 11, 2015
@hueniverse hueniverse assigned Marsup and unassigned hueniverse Feb 20, 2015
@hueniverse hueniverse added this to the 8.3.0 milestone Feb 20, 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.

Loading

@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.

7 participants