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

Ability to create an adapter dynamically #124

Open
ceilfors opened this issue Jun 22, 2019 · 8 comments

Comments

@ceilfors
Copy link
Member

@ceilfors ceilfors commented Jun 22, 2019

There are a lot of cases where users are not able to use laconia's built-in adapters. For example when creating an API endpoint, users are unable to create PUT or PATCH endpoint with the built-in adapters as we only support the inputType of body or params at the moment i.e. PUT might require body and params.

The current adapter for params is also forcing an object to be passed as an input, and sometimes destructuring just a single parameter feels quite unnatural. For example if a user is trying to get id path parameter, it will look like:

const app = ({ id }) => {}

Sometimes I'd like to have complete control over the signature of my app, and if I would like my signature like the following, I won't be able to use the current built-in adapter:

const app = (id) => {}

In Java world, this can be solved by using annotation, which may look like the following.

const app = (@PathVariable('id') id, @Body automaticallyParsedHttpBody, @PathVariable('foo') foo) => {}

As this is unsupported in JavaScript, how can we allow users to do this? Thoughts?

@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Jun 25, 2019

I have just discovered nestjs and really liked the decorator concept that it is using for JavaScript.

See this example code from nestjs

import { Controller, Get, Post, Body, Bind, Dependencies } from '@nestjs/common';
import { CatsService } from './cats.service';

@Controller('cats')
@Dependencies(CatsService)
export class CatsController {
  constructor(catsService) {
    this.catsService = catsService;
  }

  @Post()
  @Bind(Body())
  async create(createCatDto) {
    this.catsService.create(createCatDto);
  }

  @Get()
  async findAll() {
    return this.catsService.findAll();
  }
}

If we adopt a similar concept in laconia, we would be able to encourage developers to design their application first, and think about laconia later which is perfect. It might look like this:

const config = require("@laconia/config");

@Dependencies("mySecret", CatsService) // Sequence matters here. String for name based injection, constructor for type based injection. We can start with just names first which is what we support as of now
class MyAppFunction {
  constructor(secret, catsService) {
    this.secret = secret
    this.catsService = catsService;
  }

  @Function() // Let Laconia know which method to be called upon handler call
  @Adapter(ApiGateway.HTTP) // Let Laconia know which adapter should be used
  @Bind(@Body(), @Param('id')) // Sequence matters here. Adapter will parse the event and inject the request body and id parameter to the method
  execute(input, id) {
    console.log(input, id, catsService) // do something with input and dependencies
  }
}

const handler = laconia(MyAppFunction)
  .register([config.envVarInstances(), instances])

A couple of caveats:

  • Babel must be introduced as it's still in stage 2
  • Class must be used as the decorators can't be used against plain function?
  • Unlike TypeScript, the decorators can't be used on parameter level, which why nestjs is using @Dependencies and @Bind.

@laconiajs/contributors Any thoughts on the design and caveat above? Has anyone used JavaScript decorators before?

@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Jun 26, 2019

After a debate with @uncinimichel, I have counter-arguments for the usage decorator:

  • It's not appropriate to be used in application / use case level. If the principle is for the application core to be reused by multiple adapters, putting @Adapter will pollute the application.
  • Decorators probably is appropriate to be used in controller / handler level, but not application

Looking at the code again, it seems like the key concept in nestjs is the @Bind, which is something that we can introduce in the adapter level:

const app = (id, foodOrder) => {}

const apigateway = adapterApi.apigateway({
  binding: [
    adapterApi.apigateway.binding.pathVariable('id'),
    adapterApi.apigateway.binding.body
  ] // Similar to react prop types
});

exports.handler = laconia(apigateway(app)).register(instances);

We can also actually just let the users decide fully on the mapping by providing a function:

const app = (id, name) => {}

const apigateway = adapterApi.apigateway({
  mapping: (parsedRequest) => [parsedRequest.params.id, parsedRequest.body]
);

exports.handler = laconia(apigateway(app)).register(instances);
@ljcoomber

This comment has been minimized.

Copy link
Contributor

@ljcoomber ljcoomber commented Jun 29, 2019

Is the use of destructuring assignment that unnatural? I guess it's quite new but I would expect it to become increasingly familiar with JS devs. I would have thought that adding decorators was much more unnatural for JS devs and currently adds another dependency.

I like the idea of user defined mappings for those that don’t want to use destructuring. It’s explicit and intuitive for anyone who knows vanilla-JS.

Native support for decorators is coming soon so think it would be worth re-examining it then. Given the proposal appears to be syntactic sugar for calling an unary-function, I wonder if there is a way of adding this capability so that the same approach can be used for explicit decoration and use of the annotation?

@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Jul 1, 2019

On the point of being natural or not, I guess I wonder how much of nestjs users are actually using the JS version of its capability (versus TypeScript). I have to agree with you on this one that it's quite unnatural, I'm quite biased with my Java background.

On the native support and dependency, nestjs is using babel to transpile the decorator, so the dependency is not actually needed in runtime. To make it super easy, they also have a start kit i.e. one click bootstrap for JS projects.

That's an interesting idea to support it with explicit-decorator now and support annotated-decorator later. Just to clarify my term, it may look like this:

Explicit-decorator binding:

const bind = bind([
    param('id'),
    body()
  ])
const apigateway = bind(adapterApi.apigateway({
  binding: 
}));

Annotated-decorator binding:

@Bind(@Body(), @Param('id'))

I wonder if the explicit-decorator brings much clarity as compared to the annotated ones that it's worth doing.

One of the benefits of the binding mechanism is it's declarative. Being declarative may bring other benefits when it's a typed language, how much value will it bring in JS though?

What are the disadvantages of the imperative mapping approach?

const apigateway = adapterApi.apigateway({
  mapping: (parsedRequest) => [parsedRequest.params.id, parsedRequest.body]
);
@ljcoomber

This comment has been minimized.

Copy link
Contributor

@ljcoomber ljcoomber commented Jul 4, 2019

Fair enough on the dependency point. But I don't class requiring Babel to use Laconia being much of an improvement ;-) (though I guess it could be optional so only if you want the decorator annotations?)

@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Jul 5, 2019

Agreed, maybe this is too much of an overkill. Seems like the imperative approach would work. Are there disadvantages that we can think of?

@hugosenari

This comment has been minimized.

Copy link
Contributor

@hugosenari hugosenari commented Sep 16, 2019

add .on("mapping", parsedRequest => [parsedRequest.params.id, parsedRequest.body]) to #116 ;-)

@ceilfors

This comment has been minimized.

Copy link
Member Author

@ceilfors ceilfors commented Sep 16, 2019

@hugosenari Thanks for contributing here. I feel that .on() is more suitable towards events, and the mapping doesn't feel like one of it. It feels a little unnatural to include a logic that we will apply by method chain, unless the entire flow is managed by a method chain e.g. ['an', 'array'].filter().map().reduce().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.