feat: http method specification per handler

tomassasovsky commented 2 years ago

Description

There should be a way to define method-based callbacks. I was thinking of a couple of ways to possibly do this:

Filenames; we could use file-naming conventions that contain the method used for the callback in it.

routes
└───user
│   │   get.dart
│   │   post.dart
└───photos
│   │   [photoId]get.dart
│   │   post.dart
│
...

Different callbacks; maybe different callbacks could be registered for each method available:


Future<Response> onGetRequest(RequestContext context) async {
return Response(body: 'this is a GET method');
}

Future onPostRequest(RequestContext context) async { return Response(body: 'this is a POST method'); }

Future onPutRequest(RequestContext context) async { return Response(body: 'this is a PUT method'); }

Future onDeleteRequest(RequestContext context) async { return Response(body: 'this is a DELETE method'); }



This is what I came up with, but if you think of something better I'd love to hear it :))

**Requirements**

- [ ] codegen-method-based mapping to a callback
- [ ] If, say, an endpoint with a GET method exists but it gets called with a POST method, it will return a 404 NOT FOUND.

felangel commented 2 years ago

Hi @tomassasovsky 👋 Thanks for opening an issue!

This is one of the next items on the roadmap and we’re currently evaluating multiple approaches. We’ll provide updates sometime next week 👍

felangel commented 2 years ago

Proposal to support specific HTTP Method handlers

Supporting ALL HTTP Methods

By default any dart file in routes is considered a route handler for all http methods (no change from the current behavior).

For example given the following file structure:

── routes
│   ├── echo
│   │   ├── [message].dart
│   │   └── _middleware.dart
│   ├── user.dart
│   └── index.dart

The server would support the following endpoints:

ALL /echo/<message>
ALL /user
ALL /

Where ALL means all HTTP methods will be routed to the respective onRequest handler.

Supporting Specific HTTP Methods

In order to register route handlers for specific HTTP methods, I propose using a method prefix on the route file names to denote that a route handler should only apply for a specific HTTP method.

The syntax for the http method would look like:

$<METHOD>_ where <METHOD> can be one of the following:

delete -> handle HTTP DELETE requests
get -> handle HTTP GET requests
patch -> handle HTTP PATCH requests
post -> handle HTTP POST requests
put -> handle HTTP PUT requests

For example given the following file structure:

── routes
│   ├── echo
│   │   ├── $get_[message].dart
│   │   └── _middleware.dart
│   ├── $delete_user.dart
│   ├── $get_user.dart
│   └── index.dart

The server would support the following endpoints:

GET /echo/<message>
DELETE /user
GET /user
ALL /

Rationale

By using a file name prefix, we minimize the impact on the code generation in terms of performance (files contents do not need to be analyzed) and we also end up with file names that more accurately reflect the functionality of the handler (e.g. get message, get user, and delete user). In addition, handlers for different HTTP methods usually have distinct functionality so this approach would encourage splitting independent handler logic into separate files which should make maintaining the code easier in larger scale projects.

Backward compatibility is preserved and developers can still choose to have a single route handler that handles all http methods and switch on the method:

Response onRequest(RequestContext context) {
  switch (context.request.method) {
    case HttpMethod.get:
      return _onGetRequest(context);
   ...
  }
}

If you have any feedback please either 👍 or 👎 this proposal and leave a comment with any additional context, thanks! 🙏

juampiq6 commented 2 years ago

I find it kind of awkward that, for example, the index should be $getindex.dart. Maybe moving the http method part to the end of the name would make more sense? index$get.dart, user$get.dart, user$delete.dart?

felangel commented 2 years ago

I find it kind of awkward that, for example, the index should be $getindex.dart. Maybe moving the http method part to the end of the name would make more sense? index$get.dart, user$get.dart, user$delete.dart?

Thanks for the feedback!

Can you elaborate a bit on what feels awkward? To me,$get_user feels more clear than user_$get because it reads like "get user" which presumably describes the functionality of the respective route handler as opposed to "user get". In addition it aligns more closely with how a cURL is structured (HTTP method first)

GET /user HTTP/1.1

pretorean commented 2 years ago

I think the $get_user option is not convenient because the IDE usually sorts files alphabetically. It is convenient to see all user_$%method% routes next to each other.

orestesgaolin commented 2 years ago

Personally I'd fell more comfortable by having all (GET, UPDATE, PUT, POST...) methods in the same file instead of splitting them between multiple ones, especially if the name would contain $ which is uncommon character. Having a breaking change by either introducing separate methods or different request/context types would be OK for me taking into account how early in the development phase we are.

felangel commented 2 years ago

Personally I'd fell more comfortable by having all (GET, UPDATE, PUT, POST...) methods in the same file instead of splitting them between multiple ones, especially if the name would contain $ which is uncommon character. Having a breaking change by either introducing separate methods or different request/context types would be OK for me taking into account how early in the development phase we are.

That approach will incur a performance penalty because we will need to read the contents of all route handlers as part of the codegen phase.

tomassasovsky commented 2 years ago

Personally I'd fell more comfortable by having all (GET, UPDATE, PUT, POST...) methods in the same file instead of splitting them between multiple ones, especially if the name would contain $ which is uncommon character. Having a breaking change by either introducing separate methods or different request/context types would be OK for me taking into account how early in the development phase we are.

I think keeping them in separate files will help with readability, especially when the logic gets complex in the requests because the files will start to be very big. (JMHO)

mtwichel commented 2 years ago

What if to get specific http methods, you have to nest them in a folder similarly to how you can nest index.dart? For example:

─routes
│  ├─user
│     ├─$get.dart
│     ├─$post.dart
│     └─_middleware.dart

Would support: GET /user POST /user

This would have the effect of:

Maintain complete file based routing - thus no performance losses (per https://github.com/VeryGoodOpenSource/dart_frog/issues/57#issuecomment-1144217335)
Would group all the functions for a given route together (per https://github.com/VeryGoodOpenSource/dart_frog/issues/57#issuecomment-1144202421)
Avoids any potential readability concerns (per https://github.com/VeryGoodOpenSource/dart_frog/issues/57#issuecomment-1144023639)

A potential drawback I see is there does have to be more nesting of files. While I don't especially love that, I think for a file-based routing system that's completely appropriate.

felangel commented 2 years ago

What if to get specific http methods, you have to nest them in a folder similarly to how you can nest index.dart? For example:
─routes
│  ├─user
│     ├─$get.dart
│     ├─$post.dart
│     └─_middleware.dart
Would support: GET /user POST /user

This would have the effect of:

Maintain complete file based routing - thus no performance losses (per feat: http method specification per handler #57 (comment))

Would group all the functions for a given route together (per feat: http method specification per handler #57 (comment))

Avoids any potential readability concerns (per feat: http method specification per handler #57 (comment))

A potential drawback I see is there does have to be more nesting of files. While I don't especially love that, I think for a file-based routing system that's completely appropriate.

Thanks for the feedback!

I think we can support both options so that the following are equivalent:

─routes
│  └─user
│     ├─$get.dart
│     └─$post.dart

─routes
│  ├─$get_user
│  └─$post_user.dart

This is similar to how you can currently do:

─routes
│  └─user
│     └─index.dart

or

─routes
│  └─user.dart

mtwichel commented 2 years ago

I think we can support both options so that the following are equivalent

That sounds like the best option to me 🎉 That way, people can group if they need to, but they can also have less nesting if that's their jam. Put the choice on the person making the API, not enforced by the framework 💙

dancamdev commented 2 years ago

Personally, I don't like to rely on file names for this because they're prone to typos and no linter can help you catch errors. You may be left wondering why the endpoint isn't working to figure out you have a file name wrong eventually.

I like the following approach better, using decorators:

@Post
Response onRequest(RequestContext context) {}

Or possibly, even:

@Methods([HttpMethod.get, HttpMethod.post])
Response onRequest(RequestContext context) {
    // switch statement here.
}

juampiq6 commented 2 years ago

I find it kind of awkward that, for example, the index should be $getindex.dart. Maybe moving the http method part to the end of the name would make more sense? index$get.dart, user$get.dart, user$delete.dart?

Thanks for the feedback!

Can you elaborate a bit on what feels awkward? To me,$get_user feels more clear than user_$get because it reads like "get user" which presumably describes the functionality of the respective route handler as opposed to "user get". In addition it aligns more closely with how a cURL is structured (HTTP method first)
GET /user HTTP/1.1

I think the conversation flowed other way haha, but I was being based on folder structure. So in my head, having the full path endpoint and the method to end it, would make more sense. ─routes │ └─user │ ├─$get.dart │ └─$post.dart

I think this a better way to handle it, not the way I have stated above. Also finding the endpoint would be quicker (supposing that a folder had a lot of "leaf" endpoints with their method attached in their name.

felangel commented 2 years ago

Personally, I don't like to rely on file names for this because they're prone to typos and no linter can help you catch errors. You may be left wondering why the endpoint isn't working to figure out you have a file name wrong eventually.

I like the following approach better, using decorators:
@Post
Response onRequest(RequestContext context) {}
Or possibly, even:
@Methods([HttpMethod.get, HttpMethod.post])
Response onRequest(RequestContext context) {
    // switch statement here.
}

Thanks for the feedback!

As previously mentioned, codegen performance will take a hit due to needing to read all file contents.

In addition, the issue with the first approach is there is no way to specify multiple handlers for multiple methods

@Post
Response onRequest(RequestContext context) {...}

// How can I register a different handler for GET requests?

The option you suggested works but it requires switch statements which is already possible and you'd still be forced to handle every case even though they wouldn't all occur:

@Methods([HttpMethod.get, HttpMethod.post])
Response onRequest(RequestContext context) {
    switch (context.request.method) {
      case  HttpMethod.get:
        return _onGetRequest(...);
      case HttpMethod.post:
        return _onPostRequest(...);
      default:
        // we still need to have a default even though this code path is unreachable.
        return Response(statusCode: 405);
    }
}

This is arguably worse than the current API without any annotations.

Another option would be to support arbitrary method names with annotations:

@Post
Response methodA(RequestContext context) {...}

@Get
Response methodB(RequestContext context) {...}

This also poses an issue because there's no way to enforce an annotation isn't used multiple times:

// What should happen here? Should we error? Handle the first? Second? It becomes ambiguous.
@Post
Response methodA(RequestContext context) {...}

@Post
Response methodB(RequestContext context) {...}

As a result, I think the most robust option if we want to support multiple handlers in a single file is to do it based on method name:

Response onGetRequest(RequestContext context) {...}
Response onPostRequest(RequestContext context) {...}
Response onPutRequest(RequestContext context) {...}

But this makes it inconvenient if you want to have a single request handler for multiple HttpMethods and is incompatible with the current onRequest API.

We could support both but onRequest would only be called if there isn't an explicit handler for the specific HttpMethod which could be confusing/unintuitive.

/// Called for all requests except GET.
Response onRequest(RequestContext context) {...}

/// Called for only get requests.
Response onGetRequest(RequestContext context) {...}

I also think the biggest argument in favor of handling different http methods in different files is for maintainability. Usually logic for handling a GET is quite different from logic for handling a POST and it generally seems like a good idea to split that functionality out into separate files rather than having a single bloated file.

Let me know what you think and thanks again for providing feedback!

dbilgin commented 2 years ago

/// Called for all requests except GET.
Response onRequest(RequestContext context) {...}

/// Called for only get requests.
Response onGetRequest(RequestContext context) {...}

I honestly don't think this could be confusing or unintuitive for anyone as it is the default thought process to think that you need to declare the verbs you want to use for the endpoints after all. So if there is none, then it's the odd one out which would be my first guess that it is going to be the "default" handler for that endpoint. onRequest itself sounds like "on any request that's coming this way" to me.

Even though it's something I am not used to to declare the routes through file/folder names, I got used to that pretty quickly playing around with dart_frog, but I feel like adding the logic for the verbs to file names as well would be a bit of an overkill and possibly a bit overwhelming for any new starters. I remember you mentioned the aim for dart_frog was to be a light, simple framework like express on the stream recently, so I think this would be a step towards making it tad more complicated to get used to/learn.

Of course I could be wrong but this was the initial feeling I got from this.

felangel commented 2 years ago

Even though it's something I am not used to to declare the routes through file/folder names, I got used to that pretty quickly playing around with dart_frog, but I feel like adding the logic for the verbs to file names as well would be a bit of an overkill and possibly a bit overwhelming for any new starters. I remember you mentioned the aim for dart_frog was to be a light, simple framework like express on the stream recently, so I think this would be a step towards making it tad more complicated to get used to/learn.

Just wanted to note that the changes proposed would be fully backward compatible so if you preferred to just have an onRequest method and use a switch statement you could continue to do so and would never need to add http methods to the file name.

tomassasovsky commented 2 years ago

Personally, I don't like to rely on file names for this because they're prone to typos and no linter can help you catch errors. You may be left wondering why the endpoint isn't working to figure out you have a file name wrong eventually.

I like the following approach better, using decorators:
@Post
Response onRequest(RequestContext context) {}
Or possibly, even:
@Methods([HttpMethod.get, HttpMethod.post])
Response onRequest(RequestContext context) {
    // switch statement here.
}

@felangel I think the VSCode Extension could handle file creation to avoid typos? So that way, it won't be an issue anymore.

dancamdev commented 2 years ago

I wouldn't rely on an extension to get this right:

it is still prone to errors (e.g. renaming files)
maintaining the extension would become crucial to have anyone working with dart-frog.
not everyone uses vs code, and having to maintain extensions and plugins for each IDE can be difficult.

I like the code based approach way more, even with different methods handling different requests. It doesn't brake anything and gives full flexibility, while guaranteeing the code works upon compilation.

tomassasovsky commented 2 years ago

I wouldn't rely on an extension to get this right:
* it is still prone to errors (e.g. renaming files)

* maintaining the extension would become crucial to have anyone working with dart-frog.

* not everyone uses vs code, and having to maintain extensions and plugins for each IDE can be difficult.
I like the code based approach way more, even with different methods handling different requests. It doesn't brake anything and gives full flexibility, while guaranteeing the code works upon compilation.

I get that this solution is still prone to errors, but (IMO) any solution will be to some degree or another. If we base it on different methods, there will be no way of ensuring that those methods are correctly named, will there?

And about having to use VSCode for this, I think a nice workaround would be adding the same functionality in the CLI tool. That way, even if you use another code editor/IDE, you'll still be able to handle this externally and in an automated way.

Luckey-Elijah commented 2 years ago

Could a wrapping handler be written for requests? Something like:


typedef MethodHandler = FutureOr<Response> Function(RequestContext)?;
typedef NoMethodHandler = FutureOr<Response> Function(RequestContext)?;

FutureOr<Response> methodRequestHandler(
  RequestContext context, {
  NoMethodHandler onUnhandledMethod,
  MethodHandler onDelete,
  MethodHandler onGet,
  MethodHandler onHead,
  MethodHandler onOptions,
  MethodHandler onPatch,
  MethodHandler onPost,
  MethodHandler onPut,
}) {
  final completer = <HttpMethod, MethodHandler>{
    HttpMethod.delete: onDelete,
    HttpMethod.get: onGet,
    HttpMethod.head: onHead,
    HttpMethod.options: onOptions,
    HttpMethod.patch: onPatch,
    HttpMethod.post: onPost,
    HttpMethod.put: onPut,
  };

  final execution = completer[context.request.method] ??
      onUnhandledMethod ??
      (_) => throw UnimplementedError(
            'No handlers for this route were registered.',
          );

  return execution(context);
}

And using it like:

Future<Response> onRequest(RequestContext context) async =>
    methodRequestHandler(
      context,
      onGet: (context) => Response.json(
        body: {'message': 'Hello, GET request!'},
      ),
      onUnhandledMethod: (context) => Response.json(
        statusCode: 404,
        body: {
          'message': '${context.request.method.value} request is not supported.'
        },
      ),
    );

felangel commented 2 years ago

@Luckey-Elijah we can do that but I’m not sure it adds much value over the current approach:

Future<Response> onRequest(RequestContext context) async {
  switch (context.request.method) {
    case HttpMethod.get:
      return _onGetRequest(context);
    case HttpMethod.post:
      return _onPostRequest(context);
    …
  }
}

wdyt?

Luckey-Elijah commented 2 years ago

I think it becomes more concise without sacrificing readability.

Future<Response> onRequest(RequestContext context) async =>
  methodRequestHandler(
    context,
    onGet: _onGetRequest,
    onPost: _onPostRequest,
  );

versus

Future<Response> onRequest(RequestContext context) async {
  switch (context.request.method) {
    case HttpMethod.get:
      return _onGetRequest(context);
    case HttpMethod.post:
      return _onPostRequest(context);
    …
  }
}

dancamdev commented 2 years ago

I think it becomes more concise without sacrificing readability.

Future<Response> onRequest(RequestContext context) async =>
  methodRequestHandler(
    context,
    onGet: _onGetRequest,
    onPost: _onPostRequest,
  );

versus

Future<Response> onRequest(RequestContext context) async {
  switch (context.request.method) {
    case HttpMethod.get:
      return _onGetRequest(context);
    case HttpMethod.post:
      return _onPostRequest(context);
    …
  }
}

I like this also because it doesn't force you to handle all of the cases, you can always have a default within the switch but this is more readable to me.

dbilgin commented 2 years ago

I believe this is a very small improvement in readability compared to switch-case, what would be the gain compared to just having separate named functions at this point?

Luckey-Elijah commented 2 years ago

I believe this is a very small improvement in readability compared to switch-case, what would be the gain compared to just having separate named functions at this point?

The biggest gain is less boilerplate compared to other options, which is for some people is not a huge and I can respect that. But let me propose why this reduction of boilerplate is helpful.

Let's say you need to implement a route that must handle GET and POST requests and return 404+static message for all other methods. Also, all three of the handlers are all ready written (onUnhandledMethod, onGetRequest, and onPostRequest) and now you only need to invoke them.

Here are several ways to do the implementation and some thoughts of mine on each

Option A: `methodRequestHandler`

```dart Future onRequest(RequestContext context) async { return methodRequestHandler( context, onGet: onGetRequest, onPost: onPostRequest, onUnhandledMethod: onUnhandledMethod, ); } ```

Option B: decorators/code generation

I think decorators and code generation were already discussed a bit, and I am on the side that it is more boilerplate and unclear what how this would be executed/implemented. ```dart @SupportedMethods([HttpMethod.get, HttpMethod.post]) Future onRequest(RequestContext context) async { // TODO: Implement GET and POST separately somehow? } ```

Option C: Separately name functions

I like this a lot. I just don't think that there might be a little bit of confusion on what would happen if you also included the `onRequest` function. ```dart Future onGetRequest(RequestContext context) async {...} Future onPostRequest(RequestContext context) async {...} Future onUnhandledRequest(RequestContext context) async {...} ```

Option D: Switch Statement with no `default` and no "Missing case clause"

A lot of boilerplate. ```dart Future onRequest(RequestContext context) async { switch (context.request.method) { case HttpMethod.get: return onGetRequest(context); case HttpMethod.post: return onPostRequest(context); case HttpMethod.delete: case HttpMethod.head: case HttpMethod.options: case HttpMethod.patch: case HttpMethod.put: return onUnhandledMethod(context); } } ```

Option E: Switch Statement with `default`

If you have enabled [`no_default_cases`](https://dart-lang.github.io/linter/lints/no_default_cases.html), this would be a problem. ```dart Future onRequest(RequestContext context) async { switch (context.request.method) { case HttpMethod.get: return onGetRequest(context); case HttpMethod.post: return onPostRequest(context); default: // No default cases lint return onUnhandledMethod(context); } } ```

Option F: Switch Statement with "Missing case clause"

analysis warning: [`missing_enum_constant_in_switch`](https://dart.dev/tools/diagnostic-messages?utm_source=dartdev&utm_medium=redir&utm_id=diagcode&utm_content=missing_enum_constant_in_switch#missing_enum_constant_in_switch) ```dart Future onRequest(RequestContext context) async { switch (context.request.method) { // Missing case clause for lint case HttpMethod.get: return onGetRequest(context); case HttpMethod.post: return onPostRequest(context); } return onUnhandledMethod(context); } ```

Option G: if-else

```dart Future onRequest(RequestContext context) async { if (context.request.method == HttpMethod.get) { return onGetRequest(context); } else if (context.request.method == HttpMethod.post) { return onPostRequest(context); } return onUnhandledMethod(context); } ```

Option H: name of file based route

This is an interesting option but seems like the most boilerplate in my opinion. ``` routes/ ├── $get_route.dart ├── $post_route.dart └── $unhandled_route.dart ``` ```dart // $get_route.dart Future onRequest(RequestContext context) async {...} ``` ```dart // $post_route.dart Future onRequest(RequestContext context) async {...} ``` ```dart // $unhandled_route.dart Future onRequest(RequestContext context) async {...} ```

I think Option A and Option C are less "boiler-platey" (and the most concise & readable).

(Please let me know If I'm missing something. I think this is great discussion 😄).

felangel commented 2 years ago

Future<Response> onRequest(RequestContext context) async =>
  methodRequestHandler(
    context,
    onGet: _onGetRequest,
    onPost: _onPostRequest,
  );

versus

Future<Response> onRequest(RequestContext context) async {
  switch (context.request.method) {
    case HttpMethod.get:
      return _onGetRequest(context);
    case HttpMethod.post:
      return _onPostRequest(context);
    …
  }
}

I think it's just a personal preference at this point. Developers are already familiar with switch statements and this is already supported with the existing implementation. In my opinion, it's just syntactic sugar which I personally don't like because it exposes multiple ways to achieve the same thing (developers can always choose to use a switch statement) and the added benefit is just a matter of preference (helper functions vs switch cases).

In terms of the proposed solutions, I'm curious to hear more about why you feel option H is the most boilerplate. Eventually the CLI would support generating new routes and middleware automatically (and we'd even add IntelliJ/VSCode support) so you'd never need to manually create the files by hand. In addition, the structure is a lot more maintainable imo as each http method likely has a drastically different function (reading data is quite different from mutating data).

It's advantageous to split functionality that's independent into separate files/methods so that they can be maintained, tested, and iterated on independently of each other. Thanks again for the feedback, I really appreciate the discussions! 🙏

Luckey-Elijah commented 2 years ago

I think it's just a personal preference at this point. Developers are already familiar with switch statements and this is already supported with the existing implementation. In my opinion, it's just syntactic sugar which I personalyl don't like because it exposes multiple ways to achieve the same thing (developers can always choose to use a switch statement) and the added benefit is just a matter of preference (helper functions vs switch cases).

Yes, you are absolutely right. "syntactical sugar" and "personal preference" is exactly what it is.

I'm curious to hear more about why you feel option H is the most boilerplate.

You need to create new, correctly named files for a route for each new method. But it does makes sense so.

Eventually the CLI would support generating new routes and middleware automatically

I think this (the quality of the CLI) would compensate for the how much boilerplate any solution would do. I didn't even consider the CLI earlier — which makes the most sense to leverage.

juampiq6 commented 2 years ago

I think the 'one file per http method' approach is the best. Having only one file per route for all http method doesn't scale IMHO. Imagine if each method occupies 100 lines (dart formatter is very short line configured). That would lead to 400 lines for a normal route which has GET, POST, DELETE and PUT (basic CRUD endpoint). Plus, version control aka git, works better (leads to less problems) if you have more files than a lot of lines in few files :) (also to mention the parallel working capacity).

On Tue, Jun 7, 2022, 1:56 PM Felix Angelov @.***> wrote:

Future onRequest(RequestContext context) async =>

methodRequestHandler(
context,

onGet: _onGetRequest,

onPost: _onPostRequest,
);

versus

Future onRequest(RequestContext context) async {

switch (context.request.method) {
case HttpMethod.get:

  return _onGetRequest(context);

case HttpMethod.post:

  return _onPostRequest(context);

…
}

}

I think it's just a personal preference at this point. Developers are already familiar with switch statements and this is already supported with the existing implementation. In my opinion, it's just syntactic sugar which I personal don't like because it exposes multiple ways to achieve the same thing (developers can always choose to use a switch statement) and the added benefit is just a matter of preference (helper functions vs switch cases).

In terms of the proposed solutions, I'm curious to hear more about why you feel option H is the most boilerplate. Eventually the CLI would support generating new routes and middleware automatically (and we'd even add IntelliJ/VSCode support) so you'd never need to manually create the files by hand. In addition, the structure is a lot more maintainable imo as each http method likely has a drastically different function (reading data is quite different from mutating data).

It's advantageous to split functionality that's independent into separate files/methods so that they can be maintained, tested, and iterated on independently of each other. Thanks again for the feedback, I really appreciate the discussions! 🙏

— Reply to this email directly, view it on GitHub https://github.com/VeryGoodOpenSource/dart_frog/issues/57#issuecomment-1148928677, or unsubscribe https://github.com/notifications/unsubscribe-auth/AE3UPLXJJOCLVKS7IQNRHF3VN55MHANCNFSM5XCUVHMQ . You are receiving this because you commented.Message ID: @.***>

blopker commented 2 years ago

So, for what it's worth, I wanted to chime in here because I think this project is awesome and is something the dart ecosystem needs dearly, especially if we can reduce the boilerplate to work with Flutter. ❤️

To start, thank you @Luckey-Elijah for listing out all the options so far. This makes it so much easier to have a discussion.

I wanted to also lay out my own personal priorities, so it's clear what motivates my opinions: In order, I care about runtime performance, boilerplate, then build time.

For runtime performance, it's unclear that any of these options are obviously faster than any other. I would like to know if that is not the case. I also don't see any build time issues for each option, except for the on[METHOD]Request one. Personally I've never seen codegen be slow enough in our relatively large Flutter project that would make me think this would be an issue. I could be totally wrong here though.

Option H

First I'd like to address option H since it is the most novel to me. I've heard a few arguments for it:

Clean separation for the different methods
One file for all methods will get too large
The CLI will take care of all the pain of dealing with multiple files
It's the least amount of boilerplate

For 1, in my experience with "very large codebases" the assumption that there's little overlap between methods doesn't resonate with me. Take for instance a form. Both the GET and POST methods need to take the request, probably authenticate it, take in all sorts data like cookies or headers, create the form, the render HTML and return it. Only the POST method does the form validation and adds errors. In practice the form validation is usually delegated quickly to another part of the codebase since forms may show up on multiple routes. In a complex route, typically only a small part of it is dedicated to the POST-specific logic.

For 2, if a handler file gets too big, that is a code smell. Handlers should only really be used for dealing with request-specific stuff. Everything else should be delegated to the HTTP-agnostic business part of the codebase. Dart already has tools to deal with this, imports.

For 3, the CLI could deal with the boilerplate around creating the files, however there are other usability issues that the CLI may not solve, like moving files around or renaming files. These issues go away if the developer can manage using tools they already have. I personally prefer to not use CLIs for this reason. I already have a bunch of aliases and workflows for managing files. This also brings up a question about what the ethos of this framework is. Is it kitchen sink, like Ruby on Rails, or is it more no-frills like expressjs, or somewhere in the middle like nextjs? Notably nextjs, which seems closest to this project so far, doesn't have any scaffolding features in their CLI.

For 4, this option actually seems like the most boilerplate because it involves multiple files to manage. It's much easier for me to copy a snippet (especially from StackOverflow) or single file to start a new route, than to copy a few files that may not be listed next to each other. Yes, the CLI could manage some of that, but why when the alternatives don't need it?

Option H seems (to me) to add complexity and a novel (or unintuitive to me) approach that I don't think adds enough benefits to the other options.

Option B & C

Option C feels a bit more intuitive to me, but I don't think it offers enough boilerplate reduction over the simplicity of the if/switch option. It also makes it a bit harder to share logic between multiple methods. Option B feels like Java to me, but I have seen other frameworks use decorators successfully, so this feels like a taste thing. I will say the decorators in Dart are pretty limited, at least compared to Python. I tend to stay away from them.

The `onRequest` option

The option that makes most sense to me is the switch/if. This is what similar frameworks already use, like nextjs. This means less friction from people moving their projects over, which is great for getting new users in. Additionally, it opens up a path for people like @Luckey-Elijah to experiment with interesting method routing techniques while also maintaining backwards compatibility. Building on methodRequestHandler, I could also see making class-based views (ala Django) by delegating onRequest to a View class instance:

Future<Response> onRequest(RequestContext context) async => MyView().onRequest(context);

class MyView extends TemplateView {
  middleware = [authenticate, cacheControl];
  template = "index.html";
  Future<TemplateRequest> get(context) async => request.ok({'my': 'var'});
  Future<TemplateRequest> post(context) async => request.ok({'my': 'post var'});
}

This would be more cumbersome with the other options. The combination of simplicity and flexibility to let the community explore different techniques is pretty attractive to me.

TLDR; I think onRequest is good enough.

dbilgin commented 2 years ago

I think the 'one file per http method' approach is the best. Having only one file per route for all http method doesn't scale IMHO. Imagine if each method occupies 100 lines (dart formatter is very short line configured). That would lead to 400 lines for a normal route which has GET, POST, DELETE and PUT (basic CRUD endpoint). Plus, version control aka git, works better (leads to less problems) if you have more files than a lot of lines in few files :) (also to mention the parallel working capacity).

Just one small thing I wanted to add to this. I think if I were to have 400 lines of code under one onRequest I would just have a service layer at that point and put the code there rather than having it directly inside the route file.

D-E-F-G I guess we can already kind of put these options into the same basket since they are already doable and it's just a matter of "best practice" between them.

Option B I can see this working well with dart:mirrors perhaps, but I guess that's an unstable api currently as stated here. I had worked on an amateur solution on this one just out of curiosity here, but of course I have no idea about the performance impacts or the unstable condition as mentioned in the api itself.

Option A

I think it's just a personal preference at this point. Developers are already familiar with switch statements and this is already supported with the existing implementation. In my opinion, it's just syntactic sugar which I personally don't like because it exposes multiple ways to achieve the same thing (developers can always choose to use a switch statement) and the added benefit is just a matter of preference (helper functions vs switch cases).

I agree with @felangel on this one, I feel like currently this would be going the extra mile before it is even decided/realised what is preferred with the framework considering it's in the very early stages currently.

Option H I wouldn't call this any more "boilerplate" than the other options, depending on file names is definitely an interesting approach. I am thinking more on the side of having separate files is going to be an overkill. As I mentioned earlier, if the code for these calls is getting too much, it can always be split into services, we already seem to have a nice middleware support for dependency injection, so I don't think this would be a problem at all to separate such code.

Option C I might be repeating myself a bit here, but this definitely seems like the most intuitive to me. I could possibly say the same for Option B just for being used to it, but I am not even sure how it would be possible, so rather than trying to somehow completely get rid of the boilerplate, I am thinking this is the most explanatory and easy to onboard option for anyone and the ambiguity of having onRequest at the same time with for example onPostRequest would be one of the very first things that anyone would discover and clear up while trying out the frog (if they didn't read the then docs)

Thank you for the list @Luckey-Elijah

felangel commented 2 years ago

Update

It appears as though the original proposal had mostly positive feedback. Several people mentioned they preferred the current API over a file-based one but as the two are not mutually exclusive we'd like to move forward with the proposed implementation.

Open Question

There is one open question that we need to address:

We mentioned supporting an alternative shorthand syntax in addition to the proposed syntax:

─routes
│  └─user
│     ├─$get.dart
│     └─$post.dart

─routes
│  ├─$get_user
│  └─$post_user.dart

This is similar to how you can currently do:

─routes
│  └─user
│     └─index.dart

or

─routes
│  └─user.dart

However if we supported both it would be possible to define two route handlers that map to the same route:

─routes
│  └─user
│     ├─$get.dart
│     └─$get_index.dart

In this case, both files would result in a route handler for the /user endpoint which would result in a conflict.

Recommendation

I'm personally leaning towards not supporting the shorthand syntax just for consistency (index already maps to /) but would love to hear everyone's thoughts.

Tagging @mtwichel @tomassasovsky @wolfenrain @renancaraujo @orestesgaolin for visibility

blopker commented 2 years ago

I have to say, it's a little disheartening to see the issues to this approach not being addressed, and being dismissed as a preference. However, it's not my project, so I get it.

It's good that the single file method will also be supported though. 👌

felangel commented 2 years ago

I have to say, it's a little disheartening to see the issues to this approach not being addressed, and being dismissed as a preference. However, it's not my project, so I get it. It's good that the single file method will also be supported though. 👌

Can you elaborate on the issues you’re referring to? The only one I recall is sort order of files. I didn’t intend to dismiss any issues as preference so I apologize if my previous comment came across that way.

We’re not planning to change the current behavior/functionality so the proposed changes would be 100% opt in. If you never want to use them you wouldn’t have to.

blopker commented 2 years ago

Sure, I think the main sticking point is that it doesn't seem to solve the issues it's trying to tackle. I've outlined those issues in my longer comment. It will though add complexity to the codebase. I think there needs to be a strong justification for any feature that adds complexity or deviates from the norm. I just haven't seen that justification yet.

I do fully acknowledge that I'm not familiar with this framework yet and have not created an app with it. I'm drawing on my experience from other frameworks I've used, and the mapping may not be great. Anyway, I'm happy that the current way is still an option.

dbilgin commented 2 years ago

I would agree with @blopker here about the justification. I feel like adding any sort of complexity adds a learning curve as well, however small it is. Also having two solutions to one problem usually creates confusion, the problem being one of the main features.

mtwichel commented 2 years ago

This is similar to how you can currently do:
─routes
│  └─user
│     └─index.dart
or
─routes
│  └─user.dart

This is actually a good question: what happens if you define both a user/index.dart route and user.dart route? I would guess the cli just errors because of the conflict.

I bring that up because I think it helps answer this question:

However if we supported both it would be possible to define two route handlers that map to the same route:
─routes
│  └─user
│     ├─$get.dart
│     └─$get_index.dart
In this case, both files would result in a route handler for the /user endpoint which would result in a conflict.

In this case, both files would result in a route handler for the /user endpoint which would result in a conflict.

I think in this case couldn't we do the same thing? If both are defined, just display an error in the CLI?

In a similar vein, what happens here:

 ─routes
 │  └─user
 │     ├─$get.dart
 │     └─index.dart

Would the index.dart handler respond to every request except GET, or is this also just an error case? (I think it should be an error to preserve predictability, but just imho)

Personally, I think the shorthand is fine as long as the CLI is able to provide helpful messages when there is a conflict. It seems to me there are cases where the paths can conflict already, so adding these isn't a big deal.

As always, thanks for everything 💙 and I'm happy to jump on discord if any bit of this was confusing.

mtwichel commented 2 years ago

This is actually a good question: what happens if you define both a user/index.dart route and user.dart route? I would guess the cli just errors because of the conflict.

Just an FYI, I just tried this out and it looks like it prefers the nested version (user/index.dart) and ignores the user.dart file. Probably out of scope for the issue, but I think whatever we settle with http methods should be consistent with other edge cases. Personally, I think any ambiguity should immediately error with something like this.

felangel commented 2 years ago

Based on the discussion we're going to close this for now since there are some reservations about the proposal and the increase in complexity associated with the changes. We can revisit this in the future and we're happy to continue to discuss alternative proposals as more members of the community try dart frog. Thanks to everyone who provided feedback, we really appreciate it 💙

tomassasovsky commented 2 years ago

@felangel, I want to run an idea by you (anyone else, also feel free to chimb in): do you think there's any chance of this (realistically) working as an interchangeable module for dart_frog? That way, we wouldn't have to settle for any one of these options but go for the one that we like best.

felangel commented 2 years ago

@felangel, I want to run an idea through you (anyone else, also feel free to chimb in): do you think there's any chance of this (realistically) working as an interchangeable module for dart_frog? That way, we wouldn't have to settle for any one of these options but go for the one that we like best.

Yeah actually I'm thinking through some potential ways to support a plugin system and this would be one possible use-case 👍

tomassasovsky commented 1 year ago

@felangel any updates on this?

exaby73 commented 1 year ago

I'm wondering if a new package that used code-gen to achieve this would help... Let's say there is a package named dart_frog_generator. Using build_runner, we could write our code like this:

// routes/user/index.dart

import 'package:dart_frog/dart_frog.dart';
import 'package:dart_frog_annotations/dart_frog_annotations.dart';

part 'index.g.dart'; // dart_frog would ignore generated files

@Get()
Response someMethod(RequestContext context) {
  // Code here
}

@Post()
Response someOtherMethod(RequestContext context) {
  // Code here
}

@Default()
Response defaultCase() {
  return Response(statusCode: 405);
}

This would generate:

// routes/users/index.g.dart

part of 'index.dart';

Response onRequest(RequestContext context) {
  switch (context.request.method) {
    case HttpStatus.get:
      return someMethod();
    case HttpStatus.post:
      return someOtherMethod();
    default:
      return defaultCase();
  }
}

This would still allow for the current architecture of dart_frog to work, analyzing the files like it currently does. Only one condition to ignore generated files would need to be added.

The builder could analyze and throw errors if you have multiple handlers for one HTTP method. What do you think?

felangel commented 1 year ago

I'm wondering if a new package that used code-gen to achieve this would help... Let's say there is a package named dart_frog_generator. Using build_runner, we could write our code like this:
// routes/user/index.dart

import 'package:dart_frog/dart_frog.dart';
import 'package:dart_frog_annotations/dart_frog_annotations.dart';

part 'index.g.dart'; // dart_frog would ignore generated files

@Get()
Response someMethod(RequestContext context) {
 // Code here
}

@Post()
Response someOtherMethod(RequestContext context) {
 // Code here
}

@Default()
Response defaultCase() {
 return Response(statusCode: 405);
}
This would generate:
// routes/users/index.g.dart

part of 'index.dart';

Response onRequest(RequestContext context) {
  switch (context.request.method) {
    case HttpStatus.get:
      return someMethod();
    case HttpStatus.post:
      return someOtherMethod();
    default:
      return defaultCase();
  }
}
This would still allow for the current architecture of dart_frog to work, analyzing the files like it currently does. Only one condition to ignore generated files would need to be added.

The builder could analyze and throw errors if you have multiple handlers for one HTTP method. What do you think?

This would work but it would be a breaking change and also result in slower codegen times because we would need to statically analyze the contents of the route handlers.

exaby73 commented 1 year ago

Yes but this could be opt-in then :) Since the framework doesn't really change how it's routing, and code-gen is already something people expect to be slower than just regular plain old code.

VeryGoodOpenSource / dart_frog