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

Fix for @Param() decorator #213

Merged

Conversation

gregmagolan
Copy link
Contributor

I was trying out the @param() decorator with the latest code @nestjs/core@4.1.3 and found it wasn't working. Its not mentioned in your documentation at https://docs.nestjs.com/components but there is a reference to it in cats.controller.ts in the 01-cats-app example:

  @Get(':id')
  findOne(@Param('id', new ParseIntPipe()) id) {
    // logic
  }

In any case, I tracked the issue down to code in router-execution-context.ts and router-explorer.ts where callback.name was not defined for the follow two ReflectMetadata calls:

return Reflect.getMetadata(ROUTE_ARGS_METADATA, instance, callback.name);

and

return Reflect.getMetadata(PARAMTYPES_METADATA, instance, callback.name);.

This PR fixes it by passing the methodName from router-explorer.ts to the create function in router-execution-context.ts and down to these two calls which now look like:

return Reflect.getMetadata(ROUTE_ARGS_METADATA, instance, methodName);

and

return Reflect.getMetadata(PARAMTYPES_METADATA, instance, methodName);

This fixes the issue and my code which looks like the following now gets the :id parameter string from the route:

@Controller('foo')
export class FooController {
  @Get('get/:id')
  get(@Param('id') id: string): any {
    console.log(id);
    return null;
  }
}

@gregmagolan
Copy link
Contributor Author

gregmagolan commented Oct 28, 2017

In short, when my code is transpiled down to .js, the typescript function

get(@Param('id') id: string): any {
    console.log(id);
    return null;
  }

ends up a nameless function (function() {...}) so callback.name doesn't work in this case.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.951% when pulling aae4dc3 on gregmagolan:bugfix/fix-for-param-decorator into dc7869b on nestjs:master.

gregmagolan pushed a commit to gregmagolan/nest that referenced this pull request Oct 30, 2017
@gregmagolan
Copy link
Contributor Author

@kamilmysliwiec Any chance this can make it into the next version of @nestjs/core? Right now I'm using a custom dist for my project.

#221 reproduces the issue this PR fixes. Its happens when transpiling down to es5 (as I do in my project and I'm building with Bazel and it targets es5 by default and can't be changed easily). In anycase, it's good to have nest work with both ES6 and ES6.

@kamilmysliwiec
Copy link
Member

Hi @gregmagolan,
Fantastic job, thanks! However, transpiling down to es5 may cause any other unexpected issues. For example, the class-based middlewares might not work correctly. I have already pulled your repo and tested your changes, so they'll available in the incoming update, but still, you should consider (if it's possible!) to move to es6.

@kamilmysliwiec kamilmysliwiec merged commit d257441 into nestjs:master Oct 30, 2017
@lock
Copy link

lock bot commented Sep 24, 2019

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

@lock lock bot locked as resolved and limited conversation to collaborators Sep 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants