0.7.0

[pleerock]

No description provided.

[pleerock]

@19majkel94 you need to review this one.... 😮
Please don't kill me for the changes I made 🙏
Fixes #108 #96 #75 #57 #54 #21 #24 🐛

I'll also need to update the docs.

Also alternative names for ActionProperties, authorizationChecker and Authorized are welcomed. Also adding @slavafomin

[MichalLytek]

Looks great for me 😉 I wasn't able to read every line of diff but the main changes are checked.

[MichalLytek]

You have skipped 4. - use 1. as guthub markdown will transform it to auto enumerated list.

[pleerock]

yes I didnt worked on readme yet, will completely update it tomorrow and let you know once again

[MichalLytek]

Is the return type necessary there? Isn't type inference enough?

[pleerock]

just wanted to do it as an convention - to always put return types...

[MichalLytek]

npm install 😆

[pleerock]

😆

[MichalLytek]

Oh, I haven't check the last commit from an hour ago. And I don't like the idea of current user and authorization checker 😞

All of earlier commits was to simplify things, remove dead code or rarely used functionality, and now you introduce this. Authorization checker works like a regular middleware but with hardcoded support in core (which is simple and unnecessary). Current user decorator also work's like any custom decorator, like the JWT that I've done in 5 minutes for my project:

/**
 * Extends routing-controllers and socket-controllers functionality
 * and allows to inject object stored in JWT token into controller's actions.
 *
 * @param {string} propertyName name of the request property where express-jwt place JWT token object
 */
export function JWT(propertyName = "user") {
    return (object: object, methodName: string, index: number) => {
        const reflectedType = (Reflect as any).getMetadata("design:paramtypes", object, methodName)[index];

        // add special param handler to routing-controllers library
        defaultMetadataArgsStorage().params.push({
            target: object.constructor,
            method: methodName,
            index,
            type: ParamTypes.CUSTOM_CONVERTER,
            reflectedType,
            parseJson: false,
            isRequired: false,
            format: reflectedType,
            transform: (_, req) => req[propertyName]
        });
    };
}

It adds unnecessary overhead to the codebase like the interceptor which you removed 😉 I would think twice if it's really needed - maybe you can convince me with better example than this in samples directory?

I would consider changes in param decorator handling - no switch-case in core, but all logic should be in value or transform function 😉

[MichalLytek]

I would also consider one breaking change - remove @JsonController and use @Controller({ json: true }) instead, like with @Middleware. What do you think?

[idchlife]

@pleerock can I help somehow?

If you allow me in discussion...

About @currentuser and @authorize

My opinion: @authorize sounds like action should authorize user and then allow/disallow access. Also authorization is about rights-roles check, authentication on the other hand is about user existence in the application and his/hers authentication status (anonymous, authenticated)

@currentuser is a nice decorator!
But wait, why if we have req.user?
It could do some magic for variety express/koa/something else, but developer should know where to search user in chosen framework.

About @jsoncontroller - why do you both hate it? It looks great! If we would have responses in our actions (but we do not, we have res.status, res.send, res.json etc) we would have something like new Response/new JsonResponse (you do get where I got them from, yes?)

But Json is a common format to give responses and get requests in. I'm using it and it really does the trick. Giving options to @controller will make it look really uglier :(

Who knows, maybe somebody really wants XmlController in the future :D

[pleerock]

@idchlife

@authorize sounds like action should authorize user and then allow/disallow access. Also authorization is about rights-roles check, authentication on the other hand is about user existence in the application and his/hers authentication status (anonymous, authenticated)

correct.

@currentuser is a nice decorator!
But wait, why if we have req.user?

because this:

action(@CurrentUser() currentUser: User)

is more elegant then:

action(@Req() req: Request)
const currentUser = req.user;

plus @CurrentUser provides convenient { required: true } option.

maybe somebody really wants XmlController in the future :D

only if we go back to the future :)

[idchlife]

@pleerock I agree about elegance. But I discovered that my actions are becoming bloated already with @Body, @Req, @res, @Session etc. I think that's everyones personal choice of course. Having @currentuser is a nice touch to existing parameters decorators.

{ required: true } will throw error "required argument/user" if user does not exist, right? But not UnauthorizedError or smthn?

[MichalLytek]

because this:

action(@Req() req: Request)
const currentUser = req.user;

Welcome in ES6 destruction world 😉

action(@Req() { user }: Request)

I've used object destruction also on response object:

action(@Param("id") id: number, @Res() { sendFile }: express.Response)

But it complicates unit testing of controllers - you have to pass whole Response object or assert as any. So the more type-safe way is this weird thing:

@Res() { sendFile }: { sendFile: express.Response["sendFile"] })

I would love to see the parametrized version of @Req and @Res decorator, which should work like a @Session("property") decorator. So I would do this:

@Res("sendFile") sendFile: express.Response["sendFile"])

With this developers could do the req.user trick without weird syntax or custom decorators:

action(@Req("user") currentUser: User)

[MichalLytek]

And again I really don't like an idea of @CurrentUser() decorator and associated logic, I'm overwhelmed with the number of LoC which were needed to integrate this decorator and global handler function to do really simple things which can be done like this:

export function CurrentUser() {
    return registerParamDecorator({
        required: true,
        value: actionProperties => actionProperties.request.user,
    });
}

currentUserChecker handler would still be like actionProperties => actionProperties.request.user, as nobody is reinventing the wheel and do JWT/Sessions logic on their own, just use existing express-session or express-jwt middlewares from npm.

[MichalLytek]

updated readme and changelogs, I feel I miss something in changelogs...

You have good commit messages but yes, you've missed removed @EmptyResultCode decorator 😉

[pleerock]

@idchlife it will throw UnauthorizedError

@19majkel94

I would love to see the parametrized version of @Req and @res decorator, which should work like a @Session("property") decorator.

goal of routing-controllers is to avoid using req and res as much as possible :)

I would like @CurrentUser to stay. Thats very convenient feature in my opinion.

I'm overwhelmed with the number of LoC which were needed to integrate this decorator

if problem is in number of loc, don't worry... We'll need to refactor routing-controllers codebase. I already made a small refactoring, but it is not enough. Im not satisfied with routing-controllers codebase. It lacks of good abstraction and slightly complicated. We'll do that in the future.

[pleerock]

I released 0.7.0.alpha in @next. You can check that. Once I merge this I'll return back to typestack framework.

[MichalLytek]

goal of routing-controllers is to avoid using req and res as much as possible :)

Agree, but express has so many properties or getter methods on request that it's impossible to define dedicated decorator for each one. So @Req("property") would help with extracting data from request to not rely on whole Request object.

Also, there are many use-case when returning json is not enough, like sendFile which I've showed. So maybe in case of sendFile could I introduce the @SendFile method decorator which would work like @Render? It will send file with path returned from action method. Also download method could works the same.

[marshall007]

Agree, but express has so many properties or getter methods on request that it's impossible to define dedicated decorator for each one.

@19majkel94 you could just write a custom middleware function that merges all the request properties you care about into req.params. As long as that executes before your controller method, you should be able to inject them all using the single @Param() decorator.

[pleerock]

I don't like that people store in "req" whatever they want. First of all req is a type of Request and if you need to store something in there you do: (req as any).someVar = x; or some other ugly way. Second, to read it you again need to do that.

@Req("property") we can do that if people need this functionality.

regarding to @SendFile - there are probably other cases when people not only sending files, but other content too, including streaming. What @SendFile should do?

[MichalLytek]

you could just write a custom middleware function that merges all the request properties you care about into req.params (...) you should be able to inject them all using the single @param() decorator.

Wait... what?! From express doc about req.params:

This property is an object containing properties mapped to the named route “parameters”. For example, if you have the route /user/:name, then the “name” property is available as req.params.name.

So I don't think it's a good idea. Sure, you can do this, but what's the purpose? Why this:

action(@Params() params: any) {
    const userId = params.session.user.id;
}

is better than simply:

action(@Req() { hostname }: Request)

Can you tell me @marshall007 ? 😉

I don't like that people store in "req" whatever they want.

I haven't talked about this case. I use in my project the req.range() method and I have to inject whole request object to my action to use it to partial stream file. It would be nice to be able to use @Req("range") decorator and inject only that method, not the entire jungle 😜 It can't be used as a @Range(size: number) decorator because size param e.g. size of file, has to be determined inside action method.

regarding to @SendFile - there are probably other cases when people not only sending files, but other content too, including streaming. What @SendFile should do?

It should work the same way as the @Render decorator works:

@Get("/files/:id")
@SendFile(options)
getFile(@Param("id") id: number) {
    return getFilePathById(id);
}

So just calling response.sendFile() method with the value returned from action method. Because now it's hard to achieve it in routing-controllers, as res.sendFile is asynchronous with callback, so if you don't wrap it in a promise, you will get Can't set headers after they are sent error 😉

return new Promise<void>((resolve, reject) => {
    res.sendFile(filePath), (err: any) => err ? resolve() : reject(err));
});

BTW, there is a "typo" in readme - copy-paste left 😉
https://github.com/pleerock/routing-controllers#render-templates

[marshall007]

Why this: [...] is better than simply:

action(@Req() { hostname }: Request)

@19majkel94 the reason it's better to be explicit with your Param (and friends) decorators is because you're defining the structure of your API. In theory this information could be used to create REST API documentation and/or intelligent clients by reflecting over the metadata emitted by the TypeScript compiler.

For example, if the reflection metadata tells you that path is a route param and search is a query param, you could write a client that takes an object like { path, search } and intelligently builds the appropriate GET /:path?search=:search request.

I'm planning to work on tooling for extracting this metadata from routing-controllers into JSON so it can be consumed by doc generators or a client. So while there aren't super compelling reasons to decorate params properly right now, hopefully there will be soon! First step is looking into #122.

[MichalLytek]

For example, if the reflection metadata tells you that path is a route param and search is a query param, you could write a client that takes an object like { path, search } and intelligently builds the appropriate GET /:path?search=:search request.

Ok but it's not the case - hostname is not the API but the inside implementation, as the req.app property is or req.user is. But some advanced users need this to take an advantage of full powers of express.js. The same goes to all response methods like sendFile(), attachment(), etc. Shall I map them also to req.params as they are a part of REST API and should be used for swagger's doc generation?

[pleerock]

It would be nice to be able to use @Req("range") decorator and inject only that method, not the entire jungle

but what if range will use this inside, it won't work right? Overall @Req("range") looks dubious way of doing things, as for me its better to use request.range() - more clear, natural, safe and documented way of doing things

@SendFile and @Download

I think these decorators deserve separate issue and PR for them. Also better name for @SendFile. Looks really ugly...

Shall I map them also to req.params

no, you should not. Just use request as it is, whats is the problem with it. Its not a jungle, its an object with good things inside it, if you need it you should use it. You shouldn't use it only if you need stuff like query params, params etc because there is an elegant way of injecting those params. Injecting part of functions of request object isn't elegant.

[MichalLytek]

but what if range will use this inside, it won't work right?

Just use bind method 😉

export function extractReqProp<K extends keyof Request>(propName: K, req: Request): Request[K] {
    const prop = req[propName] as object|Function;
    if (typeof prop === "function") {
        return prop.bind(req);
    } else {
        return prop;
    }
}

or me its better to use request.range() - more clear, natural, safe and documented way of doing things

Yeah, it's more clear but as you said:

goal of routing-controllers is to avoid using req and res as much as possible :)

Also @marshall007 said that the goal is:

to create REST API documentation and/or intelligent clients by reflecting over the metadata emitted by the TypeScript compiler.

So range is a part of API - it's a parsed Content-Range header value used to serve partial content. I know that's not a really common case in typical REST API, but it's used in streaming services.

I think these decorators deserve separate issue and PR for them. Also better name for @SendFile. Looks really ugly...

It's just a name of Response method. More intuitive for express user than a fancy name not matching the method. But the best way would be to introduce custom method decorators which could work just like interceptor. We could then receive returned value from metod and do res.sendFile(returnedValue).

[pleerock]

goal of routing-controllers is to avoid using req and res as much as possible :)

everything is relative, right?) avoid, but not for such a dirty operations.

[marshall007]

So range is a part of API - it's a parsed Content-Range header value used to serve partial content.

@19majkel94 in that case you would want to use the @HeaderParam decorator directly, that way you could inform the doc generator/other tooling that this endpoint expects the header. I'd then use a transform (or class-validator if #122 lands) to parse the header value before it's injected into your controller method.

[MichalLytek]

to parse the header value before it's injected into your controller method

It can't be fully parsed AOT, it needs the maximum size of the resource param which is determined inside action param (find filename correlated with entity with id from param and read file stat from disk).

you would want to use the @HeaderParam decorator directly

In term of self-described API - yes, but the goal of routing-controllers and DI is to provide an method API which focus on the problem (return entity by id) not the way to achieve it (get a param, parse it to get value). It's hard to test if you have to create a http header string instead of pass simple { start, end } object`.

The same problem goes to JWT or Session with your solution - I should use @HeaderParam("Authentication") and then inside action method parse it by hand to get the current user object? It's really not developer friendly, your tool should recognize @JWT decorator and generate header description in API definition. I just receive ready for use User object so I don't have to generate JWT token string and create header string in case of unit testing. And since param decorators are for injection, I can just change in my code @Session("user") to @JWT() and the whole logic and testing stays the same, no need to change in case of authentication way.

I have an idea of @Range decorator which should be nice for API documentation generation tool and also developer-friendly in use. The sample usage would be like this:

action(@Range(options) reqRange) {
    const maxSize = getFromSomewhere();
    const range = reqRange(maxSize);
    // do something with array of parsed range from headers
}

where options could be e.g. { accept: "bytes" } or sth like that.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.