hapijs / glue

Server composer for hapi.js
Other
245 stars 62 forks source link

Prefix is passed only when plugin in manifest is an array #31

Closed timmarinin closed 8 years ago

timmarinin commented 9 years ago

If in manifest I do:

/* omitted for brevity */
plugins: [
  {
      './app/api': {
            routes: { prefix: '/api' }
      }
   }
]

routes in ./app/api are registered without prefix. But if I wrap in array, it does the right thing.

/* omitted for brevity */
plugins: [
  {
      './app/api': [{ // ←←
            routes: { prefix: '/api' }
      }] // ←←
   }
]

Needs clarifying the docs or fix to support both variants.

AdriVanHoudt commented 9 years ago

It should do the exact same thing https://github.com/hapijs/glue/blob/master/lib/index.js#L88-L105 weird

timmarinin commented 9 years ago

@AdriVanHoudt see this https://github.com/hapijs/glue/blob/master/lib/index.js#L175-L196

if plugin is not array, apply: {}, not registerOptions

also, relevant test case: https://github.com/marinintim/glue/blob/master/test/index.js#L182

AdriVanHoudt commented 9 years ago

ah yes that is it I think

timmarinin commented 9 years ago

@AdriVanHoudt as far as I can see, if obj is inside array, then it's treated as register options, otherwise as plugin options.

I'm trying straightforward hack, but tests are failing at {"who": "earth"} is not valid register options.

So, I see two solutions: a) document this [registerOptions] form b) change api to make passing options more transparent, like

plugin: {
      options: pluginOptions
      register: registerOptions
}
timmarinin commented 9 years ago

@csrl: I would be glad to pull request any of these options

AdriVanHoudt commented 9 years ago

the solution is up to @csrl

csrl commented 9 years ago

Yep, this aspect of the glue manifest has always bothered me, but I've been trying not to break manifest (major version) compatibility. I'm open to suggestions/approaches.

csrl commented 9 years ago

Perhaps with the Hapi 10 + Node 4, it might be a good time to revisit the manifest structure and break some things.

timmarinin commented 9 years ago

@csrl:

plugins: [
  {
    './app/api': {
        registerOptions: regOpts,
        pluginOptions: plugOpts
     }
  }
]

But what's the point to pass plugins as object with single key in array, to support multiple instances of the same plugin?

csrl commented 9 years ago

Prior to 2.1.0 the 'plugins' only understood an object whose key's were the plugin to load. In order to support loading multiple instances of the same plugin, the object keys' values could be an array of objects or an object. It was also the only way to provide registerOptions for the plugin as well. If the value was an object, then the object was treated as the pluginOptions, and no registerOptions could be specified.

pre 2.1.0 configuration options:

Only supports a single plugin instance, no registerOptions options available:

plugins: {
  './app/api': {
     pluginOption1: 'pluginValue1',
     pluginOption2: 'pluginValue2'
  }
}
// equivalent api call
server.register(
  {
    register: require('./app/api'), 
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  },
  {},
  callback
);

Load single instance of the plugin. registerOptions available, pluginOptions must be in 'options' sub object:

// same as above, but using multi instance plugin syntax
plugins: {
  './app/api': [{
     options: {
       pluginOption1: 'pluginValue1',
       pluginOption2: 'pluginValue2'
     }
  }]
}
// and adding registerOptions into the mix
plugins: {
  './app/api': [
    {
      registerOption1: 'regValue1',
      registerOption2: 'regValue2'
      'options': {
        pluginOption1: 'pluginValue1',
        pluginOption2: 'pluginValue2'
      }
    }
  ]
}
// equivalent api call
server.register(
  {
    register: require('./app/api'), 
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  },
  {
    registerOption1: 'regValue1',
    registerOption2: 'regValue2'
  },
  callback
);

Load multiple instances of the plugin. registerOptions available, pluginOptions must be in 'options' sub object:

plugins: {
  './app/api': [
    // instance 1
    {
      registerOption1: 'regValue1',
      registerOption2: 'regValue2'
      'options': {
        pluginOption1: 'pluginValue1',
        pluginOption2: 'pluginValue2'
      }
    },
    // instance 2
    {
      registerOption1: 'regValue1',
      registerOption2: 'regValue2'
      'options': {
        pluginOption1: 'pluginValue1',
        pluginOption2: 'pluginValue2'
      }
    }
  ]
}
// equivalent api calls
server.register(
  {
    register: require('./app/api'), 
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  },
  {
    registerOption1: 'regValue1',
    registerOption2: 'regValue2'
  },
  callback
);
server.register(
  {
    register: require('./app/api'), 
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  },
  {
    registerOption1: 'regValue1',
    registerOption2: 'regValue2'
  },
  callback
);

A possible v2.1.0 configuration equivalent of the above:

plugins: [
  // instance 1
  {'./app/api': [{
    registerOption1: 'regValue1',
    registerOption2: 'regValue2'
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  }]},
  // instance 2
  {'./app/api': [{
    registerOption1: 'regValue1',
    registerOption2: 'regValue2'
    options: {
      pluginOption1: 'pluginValue1',
      pluginOption2: 'pluginValue2'
    }
  }},
]
// same api calls as previous example
// And we can also do multi-instance without registerOptions and the extra syntax
plugins: [
  // instance 1
  {'./app/api': {
    pluginOption1: 'pluginValue1',
    pluginOption2: 'pluginValue2'
  }},
  // instance 2
  {'./app/api': {
    pluginOption1: 'pluginValue1',
    pluginOption2: 'pluginValue2'
  }},
]

I propose the following configuration syntax

to keep it very close to the server.register API:

registrations: [
  {
    plugin: './app/api',
    options: {
      registerOption1: 'regValue1',
      registerOption2: 'regValue2'
    }
  }
]
// and also
registrations: [
  {
    plugin: {
      register: './app/api',
      options: {
        pluginOption1: 'pluginValue1',
        pluginOption2: 'pluginValue2'
      }
    },
    options: {
      registerOption1: 'regValue1',
      registerOption2: 'regValue2'
    }
  }
]
harriha commented 9 years ago

:+1: for this change, I tripped over because of this one, too. Also, I found having the plugin path as a key a bit cumbersome, liking the change to make it a value instead. The more explicit the registration API the better, and following conventions of existing package is obviously a plus as well.

csrl commented 8 years ago

FYI @lloydbenson these changes are upcoming if you want to get rejoice ready for them.

csrl commented 8 years ago

I've implemented this on 'v3' branch. I'll merge to master after I get some feedback.

csrl commented 8 years ago

@harriha @devinivy @marinintim @AdriVanHoudt I'm interested in you all testing the v3 branch out and seeing if it meets your needs. Please provide feedback on the docs as well.

AdriVanHoudt commented 8 years ago

I currently do not use glue and time is a bit scarce and that one commit is too big to quickly review but I like the new format, also you made the this is what it looks like without glue specially dirty didn't you :P

devinivy commented 8 years ago

I think this looks good. One very small note is that I see hapi stylized as "Hapi" in the docs. My understanding is that it's typically written in lowercase.

The manifest format is nice, as long as you don't get the two separate (registration versus plugin) option fields mixed-up. I don't think it's a problem, though, really. This will be sufficient to implement plugin dependency ordering, whether as part of v3 or possibly v3.1.

timmarinin commented 8 years ago

Looks good to me. Commit looks good too, but didn't tested though (maybe on weekend)

csrl commented 8 years ago

@AdriVanHoudt vs using an async library or such? but, yah, not so clean to call everything directly with error handling.

@devinivy thanks I'll get the casing cleaned up. I also went back and forth on the registration versus plugin. I decided to stick with mapping the hapi API function names (eg. register)

AdriVanHoudt commented 8 years ago

@csrl mostly styling but it is fine since it proves your point :P

csrl commented 8 years ago

@AdriVanHoudt ok. I also intend it to be descriptive of the manifest entries mapping to the API calls, so I was overly verbose in order to do so.

boneskull commented 8 years ago

Did this land? I'm not sure it's working as expected (or alternatively, I misunderstood something).

// success
{
  registrations: [
    {
      plugin: './path/to/plugin',
      options: {
        routes: {
          prefix: '/api'
        }
      }
    }
  ]
}

// failure
{
  registrations: [
    {
      plugin: {
        register: './path/to/plugin',
        options: {
          routes: {
            prefix: '/api'
          }
        }
      }
    }
  ]
}
csrl commented 8 years ago

That is as expected. The second form, you are passing what should be registration options as plugin options, which is incorrect.

jberall commented 6 years ago

Is it possible to pass options and the prefix? Or do I just append the routes in my file?

I using Hapi v16.2

// failure
{
  registrations: [
    {
      plugin: {
        register: './path/to/plugin',
        options: {
          value: 'some val',
          routes: {
            prefix: '/api'
          }
        }
      }
    }
  ]
}
WesTyler commented 6 years ago

I believe the problem is that your routes option is being passed as a plugin option. Try moving the routes outside of plugin: {}:

{
  registrations: [
    {
      plugin: {
        register: './path/to/plugin',
        options: {
          value: 'some val'
        }
      },
      routes: {
        prefix: '/api'
      }
    }
  ]
}
jberall commented 6 years ago

Thanks for the prompt reply. I tried and no success. Not that it matters but I'm using Glue. I've decided to use a simple work around. I pass all the values as normal options then I use something like below.

I just had duplicating work. internals.lob_id = options.lob_id; internals.route_prefix = options.routes.prefix; /**

WesTyler commented 6 years ago

Oh, I'm sorry I left out a piece in my last example I think. Try this

{
  registrations: [
    {
      plugin: {
        register: './path/to/plugin',
        options: {
          value: 'some val'
        }
      },
      options: {
        routes: {
          prefix: '/api'
        }
      }
    }
  ]
}

(I set routes as a direct sibling of plugin. I should have set options as a direct sibling of plugin with routes as a child.)

jberall commented 6 years ago

Thanks, Perfection

lock[bot] commented 4 years ago

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.