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

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

Closed
timmarinin opened this issue Sep 9, 2015 · 28 comments
Closed

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

timmarinin opened this issue Sep 9, 2015 · 28 comments
Assignees
Labels
breaking changes Change that can breaking existing code feature New functionality or improvement
Milestone

Comments

@timmarinin
Copy link

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
Copy link
Contributor

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

@timmarinin
Copy link
Author

@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
Copy link
Contributor

ah yes that is it I think

@timmarinin
Copy link
Author

@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
Copy link
Author

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

@AdriVanHoudt
Copy link
Contributor

the solution is up to @csrl

@csrl
Copy link
Contributor

csrl commented Sep 14, 2015

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 csrl added feature New functionality or improvement help wanted breaking changes Change that can breaking existing code labels Sep 14, 2015
@csrl
Copy link
Contributor

csrl commented Sep 14, 2015

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

@timmarinin
Copy link
Author

@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
Copy link
Contributor

csrl commented Sep 18, 2015

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'
    }
  }
]

@csrl csrl added this to the 3.0.0 milestone Sep 18, 2015
@harriha
Copy link

harriha commented Oct 24, 2015

👍 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
Copy link
Contributor

csrl commented Nov 29, 2015

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

@csrl csrl self-assigned this Nov 29, 2015
@csrl csrl removed the help wanted label Nov 29, 2015
@csrl
Copy link
Contributor

csrl commented Nov 29, 2015

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

@csrl
Copy link
Contributor

csrl commented Dec 3, 2015

@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
Copy link
Contributor

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
Copy link
Member

devinivy commented Dec 3, 2015

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
Copy link
Author

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

@csrl
Copy link
Contributor

csrl commented Dec 4, 2015

@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
Copy link
Contributor

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

@csrl
Copy link
Contributor

csrl commented Dec 7, 2015

@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
Copy link
Contributor

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
Copy link
Contributor

csrl commented Feb 13, 2016

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

@jberall
Copy link

jberall commented Nov 28, 2017

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
Copy link
Contributor

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
Copy link

jberall commented Nov 28, 2017

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;
/**

  • Map the prefix to the path.

  • @param {*} route
    */
    internals.prefix_routes = (route) => {

    route.path = internals.route_prefix + route.path;
    return route;
    };
    var routes = [/* array with all your root */];

server.route(routes.map(internals.prefix_routes));

@WesTyler
Copy link
Contributor

WesTyler commented Nov 28, 2017

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
Copy link

jberall commented Nov 28, 2017

Thanks,
Perfection

@lock
Copy link

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
breaking changes Change that can breaking existing code feature New functionality or improvement
Projects
None yet
Development

No branches or pull requests

8 participants