Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat(transport-commons): Ability to register routes with custom params #2482

Merged
merged 3 commits into from
Nov 8, 2021

Conversation

vonagam
Copy link
Member

@vonagam vonagam commented Oct 27, 2021

Allows to add routes with some predetermined route params which are not placeholder based.

// context.params.route.some will be 'param' for '/some/path' 
app.routes.insert('/some/path', { service, params: { some: 'param' } });

@vonagam vonagam force-pushed the feat-routes-params branch 3 times, most recently from 0710fa6 to 44b9e66 Compare October 27, 2021 01:38
@vonagam vonagam force-pushed the feat-routes-params branch from 44b9e66 to ed65302 Compare November 7, 2021 18:30
@daffl
Copy link
Member

daffl commented Nov 7, 2021

Yep, can definitely see that being useful. I think we can also add it to the new service options (https://dove.docs.feathersjs.com/api/application.html#use-path-service-options) to make it more integrated:

app.use('/messages', new MyService(), {
  methods: [ 'get', 'doSomething' ],
  events: [ 'something' ],
  params: { something: 'here' }
});

@vonagam
Copy link
Member Author

vonagam commented Nov 7, 2021

One question that i had when looking through code:

Is it possible to register same instance of a service on different routes? This line makes me think that this possibility was thought about at some point.

If yes then it means that you can pass server options only during first use call since wrapService does not support having multiple settings stored. So if you pass options second time they will be silently ignored which is a bug.

If no then shouldn't wrapService throw an error (instead of returning) when it receives already wrapped service?

@daffl
Copy link
Member

daffl commented Nov 7, 2021

Great, thank you! This looks good to me.

Good point about re-using the options. Can we put up a test for this to see what would be a good way to go about that? The goal was basically to

  • Re-use existing services (but possibly with different options when passed)
  • Have custom or plugin services provide their own options (e.g. custom methods and events) without somebody having to explicitly define them on registration

@vonagam
Copy link
Member Author

vonagam commented Nov 7, 2021

Hm... if plugins will provide their own options using SERVICE symbol then it means that options provided on a registration will be ignored even on first one. But I think that this is a separate issue and should be handled in another PR.

Other question:
Are there any plans to use router from transport-commons for express? Right now koa and socket transports use the router, but not express, so this change will be useful for them but not for express.

@daffl daffl merged commit 497990a into feathersjs:dove Nov 8, 2021
@daffl
Copy link
Member

daffl commented Nov 8, 2021

You're right, the service options should be a different PR/issue.

The problem with changing the router in Express is that things like custom service middleware (and any other middleware matching Express routes which are not compatible with this router) won't work anymore.

@vonagam vonagam deleted the feat-routes-params branch November 9, 2021 13:43
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants