Open mrfrase3 opened 10 months ago
Thank you for putting this together. Here are some of my initial thoughts/questions:
/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.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.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.
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
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.
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.
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:
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 theServiceOptions
. 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.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:
packages/transport-commons/src/client.ts