WIP: Dynamic Method Definitions

[mrfrase3]

Summary

I started this refactor a while ago and had to pause it after the scope blew out. I thought I might put up a PR to get feedback before dedicating any further time to it.

One of the notable limitations of custom methods is that they are limited to POST requests with data only:

• This is annoying, as many of a user's hooks wont work if say you have to pass the id in data
  • Hooks not working adds larger room for error, potentially opening security vulnerabilities
• Sometimes data is irrelevant to the purpose of the method
• The user may want to specify a custom route/method
  • e.g. GET /tasks/summary
  • e.g. OPTIONS /tasks
  • e.g. PUT /files/123/upload
  • e.g. HEAD /files/123
  • e.g. POST /event/123/accept

I initially brought this up a while ago and got push-back (mainly for the last example) as actions in the route goes against the HTTP spec. Whilst technically correct, I'd also argue that exactly following the spec isn't always up to us or the developer, a lot of people don't follow this spec when doing api design, and actively making that more difficult hurts the framework. Ultimately it's up to the person implementing the method to decide if they are going to follow the spec.

Currently you tell the service to expose methods via the methods field in the ServiceOptions. This array only accepts strings, allowing no further configuration on a per-method basis. This PR gives the ability to instead pass a MethodDefinition object, where you can specify a more advanced use-case, whilst also still accepting a string which will default to the current functionality.

export interface MethodDefinition {
  key: string // the method name
  id?: boolean // whether the method accepts an id argument
  data?: boolean // whether the method accepts a data argument
  route?: string | boolean // whether to expose the method under a custom route, string will be the route, true will be the key in kebab-case, false (default) doesn't expose it (need to use headers)
  routeMethod?: string // the http method used on the route
  eventName?: string // override the event name
}

A lot of the blow-out in the scope of this is removing hard-coded references to the default methods and instead dynamically driving off of the definitions. Although I think making this switch will benefit in the long run. My main objective was to make custom methods more of a primary feature rather than an afterthought.

Still some things that need to be done that I can remember:

• eventName override
  • look into other things to do with events that can be configured
  • write tests
• transport clients
  • finish implementing these?
  • is there a cleaner way of doing packages/transport-commons/src/client.ts
  • tests
• add tests for the new util methods
• docs

[daffl]

Thank you for putting this together. Here are some of my initial thoughts/questions:

• To me things like /files/123/upload and /tasks/summary are semantically their own services. They already allow to implement any route and any GET, POST, PUT and DELETE method with it. Wouldn't this just add another way to do the same thing? I think one of the big mistakes I made especially for v5 was moving away from promoting services as a key primitive with the pre-built database adapters and resolvers.
• How would this work with sockets or any other transport that's not HTTP? I always found the transport independence a pretty important differentiator. Maybe I'm wrong. But if flexibility with implementing HTTP specific things is important, there's probably frameworks out there that do that better already.
• Specifying what should happen for things like HEAD and OPTIONS is a very good point. I believe the HTTP spec is pretty clear on what those should do and may not need service specific custom implementations but just a handler for it at the transport level.

[mrfrase3]

1. I can see that argument, I've found there's a balance/art to deciding when a custom method should be elevated to a service. We were adding services for single methods in the past and it resulted in a lot of bloat and boilerplate just to do a single action, and developer confusion as it often "feels" more like an anti-pattern.

• in our codebase we have a lot of services that are central data structures that a lot of separate workflows branch from, some of these are small and others are large. sometimes trying to trigger a flow from data manipulation is simple, but it's not obvious to the end-developer what exactly it is they need to do, you can also get accidental misfires if your not sure careful or infinite patch loops. On the other-hand writing custom services for every action would result in 50 sub services, some doing very basic tasks. custom methods hit that happy medium, but are just missing some crucial features.
• using custom services that wrap around another service can cause issues with things like feathers-vuex which tries to interact with the results as if their data that needs to be stored in a cache, you can also wind up with duplicate data and such.

2. the feathers-client ignores the custom routes and uses the header in a POST instead, the custom routes are more useful when you need to expose something specific outside of the norms. i.e. you don't want to send the file blob over the socket connection and want to manually use a fetch instead, or say you want to give users a direct link in an email that's example.com/event/123/approve?key=abc

3. The larger point is you can specify the method, I haven't actually tested or implemented those. internally the default methods are now defined using the same dynamic method definitions, so the code treats all methods the same without the need for hard-coded exceptions. We don't necessarily need to broadcast the custom routes part, its more there if an advanced user wants to hijack the centralised method configuration.

I'm not 100% married to the custom routes being supported, but it's a useful option to expose, and gives developers more flexibility in their options of implementing things.

[bertho-zero]

My 2 cents: The limitation mainly comes from having a method signature fixed to (data, params), methods are currently defined like this at the service level: methods: ['get', 'myCustomMethod'].

To modify this behavior without being too invasive we could define the methods as follows:

methods: [
  'get',
  ['myCustomMethod', ['id', 'data', 'params']]
]

If the custom method uses a signature other than (data, params), then you need to specify this argument array also for the client. It's a bit redundant at first but it avoids having a complex service discovery system.