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

Allow aliased imports for parameter decorators #365

Closed
HoldYourWaffle opened this issue Jun 14, 2019 · 5 comments
Closed

Allow aliased imports for parameter decorators #365

HoldYourWaffle opened this issue Jun 14, 2019 · 5 comments
Labels

Comments

@HoldYourWaffle
Copy link
Contributor

The decorator names for parameters are hardcoded as per this piece of code:

public Generate(): Tsoa.Parameter {
const decoratorName = getDecoratorName(this.parameter, (identifier) => this.supportParameterDecorator(identifier.text));
switch (decoratorName) {
case 'Request':
return this.getRequestParameter(this.parameter);
case 'Body':
return this.getBodyParameter(this.parameter);
case 'BodyProp':
return this.getBodyPropParameter(this.parameter);
case 'Header':
return this.getHeaderParameter(this.parameter);
case 'Query':
return this.getQueryParameter(this.parameter);
case 'Path':
return this.getPathParameter(this.parameter);
default:
return this.getPathParameter(this.parameter);
}
}

This prevents the use of imports like these:

import { Request as TsoaRequest } from 'tsoa'

I want to use such an import because having 2 Request types (tsoa and express) gets confusing and error-prone fast.
Using Express.Request for the non-decorator type as suggested in the readme is not sufficient because the global Request interface does not match the actual "filled in" Request. The global interface is only used (as far as I know) to add additional properties to the request interface using declaration merging (the full interface extends the global one).

The most obvious solution would be keeping track of the import aliases, but from what I've seen in the code this actually wouldn't be that simple (it looks like all AST nodes are just put on a big pile to be searched later).
While I was modifying some other parts of the code (PR maybe coming later, unsure if it's a good fit for the 'main' project) I encountered more issues with this way of resolving types, like the issue with duplicate model names (although a 'fix' is well documented).
Perhaps it's possible to parse aliased imports as a separate "reference" type to be resolved later, like some sort of proxy? Although such a 'proxy reference' would probably have issues with duplicate model or even alias names too....
Maybe a better long-term solution would be to abolish the current "big pile of nodes"-style TypeResolver and go with a more modular approach that properly handles these kind of things on a per-module basis.

As I said before I don't know that much about the code so this may all be total garbage. I'm not even sure how it would work exactly, but perhaps I'll have the time to investigate this further somewhere next week.

@dgreene1
Copy link
Collaborator

dgreene1 commented Aug 6, 2019

@HoldYourWaffle we should probably close this issue unless you're interested in putting a PR in. I say this because there doesn't appear a lot of interest in this feature yet (based on the lack of comments). This leads me to believe that most people are okay with using @Request() request: express.Request. Perhaps I'm missing something. How does express.Request differ from Request?

@HoldYourWaffle
Copy link
Contributor Author

HoldYourWaffle commented Aug 22, 2019

The express Request interface is designed in a way that you (or modules you installed) can add additional properties to it using declaration merging. For example, passport.js uses this to tell Typescript that it adds a user property to the request object. I need these additional properties in my controllers, so I can't use the "plain" global Request type.

I could look into creating a PR, but I don't think I'm familiar enough with this project to pull of such a big restructure in a somewhat competent way.

@dgreene1
Copy link
Collaborator

dgreene1 commented Sep 8, 2019

@HoldYourWaffle I still don't know enough about Passport.js to know the particular issue you're encountering. And I don't want to be like mean maintainers who would probably just say "create an example repo that recreates the issue." (although that would help us get to the root of your issue more quickly).

So at the moment, I'd like to suggest that you consider the opposite approach. Instead of finding a way to alias the decorator, why don't you change the type?

i.e. don't do this:

import { Request as TsoaRequest } from 'tsoa';
import { request } from 'express';
function getSomeResource(@TsoaRequest request: request): Promise<ISomeResource> {

instead, do this:

import { Request } from 'tsoa'

// merge the declaration manually
//    maybe even do this in a separate .d.ts file
import { request } from 'express';
import { IUser } from 'passport'; // <-- psuedo-code since I don't know passport.js
export type RequestWithUser = request & {
    user: IUser;
}

function getSomeResource(@Request request: RequestWithUser): Promise<ISomeResource> {

I believe the above will work for you since (I believe that) tsoa does not care about the TypeScript type if you put the @Request decorator on the method parameter. See here. Do to this, I don't believe that any change is necessary on tsoa's side. But of course, I might be wrong since I don't know how passport.js works. But I. If so, a reproduction project might be useful if you'd like additional help.

@HoldYourWaffle
Copy link
Contributor Author

I have no idea what happened, maybe VS Code or Typescript was just being buggy, but using express.Request is working for me now.

I add my own properties to express.Request like this:

declare global {
	namespace Express {
		interface Request {
			// extra properties
		}
	}
}

Previously these extra properties weren't detected when I used express.Request, fortunately it looks like this is fixed now.

I still think this should be implemented for overall developer experience, but if you think it's too much effort (I'm no expert but it definitely didn't look easy) I'd be completely fine with a 'wontfix' judgement (although it might be good to mention this in the documentation).

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants