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

Expanded registrations API. Added test. Updated docs. #2829

Merged
merged 4 commits into from Oct 9, 2015

Conversation

@arb
Copy link
Contributor

@arb arb commented Oct 6, 2015

Added more information to the plugin results of registrations.

@devinivy
Copy link
Member

@devinivy devinivy commented Oct 6, 2015

Any reason the '0.0.0' default version is necessary?

@arb
Copy link
Contributor Author

@arb arb commented Oct 6, 2015

Yes the 0.0.0 default is necessary. If a module doesn't have an attributes.pkg object or a attributes.version, then the plugin version ends up being undefined. Based on the tests and the default values in the plugin schema, I think the intent was to set the version to 0.0.0 in any instance where the version is not provided.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 6, 2015

Hmm. The schema has the '0.0.0' default. Is it not working right?

@hueniverse hueniverse self-assigned this Oct 6, 2015
@@ -264,7 +272,7 @@ internals.Plugin.prototype.register = function (plugins /*, [options], callback
Hoek.assert(item.multiple, 'Plugin', item.name, 'already registered');
}
else {
self.root._registrations[item.name] = { version: item.version };
self.root._registrations[item.name] = registrationData;
Copy link
Contributor

@AdriVanHoudt AdriVanHoudt Oct 6, 2015

why not just item as a whole?

@arb
Copy link
Contributor Author

@arb arb commented Oct 6, 2015

@hueniverse it works right if your plugin has register.attributes.pkg = {}, the the Joi default on "version" pops and defaults it to "0.0.0". In instances where you don't have anything on pkg, only the default value of {} is set to pkg; not on pkg.version.

@hueniverse
Copy link
Contributor

@hueniverse hueniverse commented Oct 6, 2015

The we need to fix the schema, not the code.

@arb
Copy link
Contributor Author

@arb arb commented Oct 7, 2015

@hueniverse updated schema so we could remove the || '0.0.0' code change.

var registrationData = {
version: item.version,
name: item.name,
multiple: item.multiple,
Copy link
Contributor

@hueniverse hueniverse Oct 7, 2015

This is redundant given you have attributes.

@arb arb force-pushed the registrations branch from 729a472 to 799613e Oct 7, 2015
@arb
Copy link
Contributor Author

@arb arb commented Oct 9, 2015

This should be good for a second review.

@hueniverse hueniverse added this to the 10.4.2 milestone Oct 9, 2015
hueniverse added a commit that referenced this issue Oct 9, 2015
Expanded `registrations` API. Added test. Updated docs.
@hueniverse hueniverse merged commit 03790cc into hapijs:master Oct 9, 2015
1 check passed
@arb arb deleted the registrations branch Oct 9, 2015
@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 issues

Successfully merging this pull request may close these issues.

None yet

4 participants