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

Allow different names and endpoints for services... #691

Closed
pkreipke opened this issue Oct 12, 2017 · 6 comments · Fixed by #697
Closed

Allow different names and endpoints for services... #691

pkreipke opened this issue Oct 12, 2017 · 6 comments · Fixed by #697

Comments

@pkreipke
Copy link

The CLI’s service generator for a Sequelize services adds a ‘name’ property to the options it passes to the service constructor.

const options = {
    name: 'items',
    Model,
    paginate
  };

  // Initialize our service with any options it requires
  app.use('/items', createService(options));

  // Get our initialized service so that we can register hooks and filters
  const service = app.service('items');
  1. However in this help page the 'name' option is never mentioned: https://docs.feathersjs.com/api/databases/sequelize.html#api .

  2. I’ve looked and stepped through the service() code and in fact it doesn’t seem to be used. Instead the leading slash is stripped off the service endpoint “/items” and into a variable location and the remaining part of the endpoint is also used as the service name.

  3. I thought the changing the line like so app.use('/restaurants', createService({name: 'item', Model, paginate} ) would allow me to mount the service on any url but it turns out that throws this error at launch:

Cannot read property 'hooks' of undefined
    at EventEmitter.module.exports
    ...

Proposed:

It would be great if the service name could actually be set by an option or argument such that a common service name and its code could be reused in multiple apps/projects and only the service's route changed.

Use case:

A reusable service "items" provides the same CRUD implementation (fields, computed fields, custom processing etc) for an "item" but each app that requires/includes the service could have the route be specific to its type/client in that particular app (/restaurants, /groceries, /movies, /todo etc ).

Notes:

  • not sure: perhaps rename the service by doing find/replace in the services[] array as long as the service's actual route isn't ret-conned from location in turn.

  • or perhaps vice versa: create service "items" first and then overwrite the endpoint with '/restaurants'. Issue Unregistering services and hooks #446 implies that maybe can only do in REST - because of needing to unbind from transports. So this might not be an option.

Related:

  • This is a similar thing: using Sequelize I'm able to have an actual DB table name be different than whatever I call the JS model by modifying the generated code. But that doesn't apply to the service's options.name field

  • issue Unregistering services and hooks #446 also talks about unregistering services after they're live but was removed from Buzzard. And I'm not looking for hot swapping, just intelligent options during setup

@daffl
Copy link
Member

daffl commented Oct 12, 2017

The path that you add in .use('/path' is the service name (internally leading and trailing slashes will be removed ). You are correct that the name is not used (or needed) anywhere else except for Knex where it is the database table name.

To avoid confusion in the future the best solution is probably to remove the name from the default template (https://github.com/feathersjs/generator-feathers/blob/master/generators/service/templates/service.js#L13) and to add a knex.js in https://github.com/feathersjs/generator-feathers/tree/master/generators/service/templates/types with the name property. A PR would be very welcome 😄

@pkreipke
Copy link
Author

I'm sure you said what you meant and but just to clarify here's what I'm hearing :-)

  • forget it: an alias for the service name that's different from the service's endpoint is not needed and that use case you gave is bunk ;-)

  • the only reason 'name' was in there was for knex, though you said you used sequelize, but here's a solution to that anyway, and we'd welcome help fixing that instead

:-) Which is all fine and I am happy to help.

But to be clear in case I wasn't, I was proposing a change to exactly what you pointed out is the case now:

The path that you add in .use('/path' is the service name (internally leading and trailing slashes will be removed )

That this be relaxed so that path and name could be separated (with path as the default) to encourage reuse.

Was that the PR you were asking for? I'd hate to begin something that wasn't agreed to.

Regards,
Per

@pkreipke
Copy link
Author

P.s. sorry - I see I wasn't explicit enough about sequelize - that's generating my Model :-o

@daffl
Copy link
Member

daffl commented Oct 12, 2017

I'm not saying is bunk, I would say I'm not sure I understand the use case. If we want different service names for different applications we just .use it under that name (which is what all the database services are doing ), no?

// app 1
app.use('/movies', new ItemService())

// app 2
app.use('/groceries', new ItemService())

If you want to alias the an existing service under a different name you can also do app.use('/movies', app.service('/items')).

@pkreipke
Copy link
Author

Good ideas - I'll try right away.

@lock
Copy link

lock bot commented Feb 7, 2019

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue with a link to this issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Feb 7, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants