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

Support inline dependencies on plugins #2405

Merged
merged 2 commits into from Feb 20, 2015
Merged

Conversation

@Marsup
Copy link
Contributor

@Marsup Marsup commented Feb 11, 2015

Fixes #2332.

I have a small doubt on L248 of this patch, server is the same as selection right ?

@hueniverse hueniverse self-assigned this Feb 16, 2015
};

Hoek.assert(item.name, 'Missing plugin name', hint);
Hoek.assert(typeof item.dependencies === 'string' ||
Copy link
Contributor

@hueniverse hueniverse Feb 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not a joi.assert here?

Loading

@Marsup
Copy link
Contributor Author

@Marsup Marsup commented Feb 20, 2015

@hueniverse like that or do you want to check the whole item object ? I don't know if doing it now would be a breaking change for some people.

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 20, 2015

This is fine, but why a breaking change for some people?

Loading

@hueniverse hueniverse added this to the 8.2.1 milestone Feb 20, 2015
hueniverse pushed a commit that referenced this issue Feb 20, 2015
Support inline dependencies on plugins
@hueniverse hueniverse merged commit 95f505a into hapijs:master Feb 20, 2015
1 check passed
Loading

selection.dependency(item.dependencies, function (server, next) {

// Deferred registration when all dependencies are resolve
Copy link
Contributor

@hueniverse hueniverse Feb 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a change in behavior. Dependency does not imply order. If you want to dictate order of registration you can't use this mechanism.

Loading

Copy link
Contributor Author

@Marsup Marsup Feb 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's an order to have here, dependencies in registration all just have to be there before the plugin begins his own registration. How is that a change if there wasn't any mechanism for that before ?

EDIT: now I get what you mean, I think people will expect that. If you declare a dependency to register a auth scheme or strategy for example, you need it to be done before you declare your own routes right ? What I did was less surprising IMO.

Loading

hueniverse pushed a commit that referenced this issue Feb 20, 2015
@Marsup
Copy link
Contributor Author

@Marsup Marsup commented Feb 20, 2015

@hueniverse breaking because depending on how we validate the object, we might reject existing configurations if it's too strict.

Loading

@osslate
Copy link

@osslate osslate commented Feb 20, 2015

Sorry about not working on this. Glad it's landed though :-)

Loading

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Feb 20, 2015

@Marsup not if we allow unknown keys :-)

Loading

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

Successfully merging this pull request may close these issues.

3 participants