Skip to content
This repository has been archived by the owner on Apr 20, 2019. It is now read-only.

closes #46 #47

Merged
merged 5 commits into from
Jan 3, 2016
Merged

closes #46 #47

merged 5 commits into from
Jan 3, 2016

Conversation

lloydbenson
Copy link
Contributor

No description provided.

@lloydbenson lloydbenson added this to the 3.0.0 milestone Nov 20, 2015
@lloydbenson lloydbenson self-assigned this Nov 20, 2015
@lloydbenson lloydbenson assigned geek and unassigned lloydbenson Dec 28, 2015
@@ -1,5 +1,8 @@
language: node_js

node_js:
- 0.10
- 4.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Quotes everywhere.


var value = manifest[key];
const value = manifest[key];
if (typeof value === 'string' &&
value.indexOf('$env.') === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

value.startsWith('$env.')

@lloydbenson lloydbenson assigned Marsup and unassigned geek Dec 30, 2015
"events": { "ops": "*" },
"config": "/log/ops.log"
}]
"registrations": [
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of registrations requiring an array of objects, you could simplify... if its a string then its the plugin name, if its an object then its the plugin with register and options.

"registrations": [
  {
    "register": "good",
    "options": { }
  },
  "lout"
]

You could take it a step further, if its an object, then the key is the plugin name, and the value are the options.

"registrations": [
  {
    "good": { // options here },
    "tv": { //options here }
  },
  "lout"
]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I think it would be a good idea to simplify the manifest (I don't know what was wrong with just plugins vs registrations before but rejoice just implements glue manifests and this is how it is currently being implemented with 3.x.

Choose a reason for hiding this comment

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

@geek: Please note that registration options (once, select, routes) need to be passed as well and they should not be in options as options are passed to the plugin.

Copy link

Choose a reason for hiding this comment

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

hapijs/glue#31 explains the change to the manifest format.

The simplest form you can pass as registrations is:

"registrations": [
  { "plugin": "good" },
  { "plugin": "tv" }
]

I'd possibly be open to a pull request to support the form of:

"registrations": [
   "good",
   "tv"
]

But I'm not sold on it yet.

Marsup added a commit that referenced this pull request Jan 3, 2016
@Marsup Marsup merged commit 96e7c21 into outmoded:master Jan 3, 2016
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants