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

Improve param decorators #34

Merged
merged 6 commits into from
Oct 11, 2018
Merged

Improve param decorators #34

merged 6 commits into from
Oct 11, 2018

Conversation

andrew-yustyk
Copy link

This PR fixes 2 bugs in ConfigParam() and Configurable() decorator

  1. ConfigParam() decorator set metadata to the target: Reflect.defineMetadata(CONFIG_PARAMS, existingParameters, target), but always retrieved it from target's property: Reflect.getMetadata(CONFIG_PARAMS, target, propertyKey). So previous metadata was deleted if decorator was called 2 times or more on the same method.

  2. Utility applyParamsMetadataDecorator() set config param to the argument only if the argument exists and it's an object (some unsafe condition as for me). So ConfigParam() decorator worked only the ConfigService() was injected on the argument's place.

But it's a fact that ConfigService may be not injected in all required places (check additional tests).
So I also added improvements to the applyParamsMetadataDecorator() utility.

P.S. Probably it is't a question to the nestjs-config and not a topic of this PR, but when Configurable() decorator applied to the controller method and it was placed to top of order - it breaks other decorators.

  @Configurable()
  @Get('cats')
  @UseInterceptors(MyInterceptor)
  public getMostLikedPosts(@ConfigParam('key') key):  {
    return [];
  }

I think it's because it replaces original method with own function.
It will be nice to have some note in the Readme that Configurable() decorator must be the last.

@coveralls
Copy link

coveralls commented Oct 8, 2018

Pull Request Test Coverage Report for Build 97

  • 11 of 11 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+3.04%) to 95.135%

Totals Coverage Status
Change from base Build 84: 3.04%
Covered Lines: 133
Relevant Lines: 133

💛 - Coveralls

@bashleigh
Copy link
Collaborator

bashleigh commented Oct 8, 2018

Could you add a note to the readme about the @Configurable decorator underneath the decorator code example please :) https://github.com/nestjs-community/nestjs-config#decorators
Is this only happening in your PR though? If not we'll investigate!

Other than that it looks pretty cool! Thanks :)

@bashleigh bashleigh added bug Something isn't working enhancement New feature or request labels Oct 8, 2018
@andrew-yustyk
Copy link
Author

Readme is updated with the description and examples.
Also I've created an issue on nextjs repository for this behavior - nestjs/nest#1180

@andrew-yustyk
Copy link
Author

andrew-yustyk commented Oct 9, 2018

Is this only happening in your PR though?

No, this is behavior of the current nestjs-config version (1.2.4) and also it reproduces in the PR.
Unfortunately I could not fix it

Copy link
Member

@shekohex shekohex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 😃

Another side note, can you just add a good tip in the ReadMe file that as a good practice, one should make the injected @ConfigParam(...) parameter in the method to be optional.
So that if he is trying to call it manually, not have to add a null or make it's type of any.

Thank you again 😃.

providers: [ComponentTest],
}).compile();
const componentTest = module.get<ComponentTest>(ComponentTest);
expect(componentTest.testConfig(null, null)).toEqual({ serverPort: 2000, stubPort: 2000 });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a side note, to get rid of null, null
You could make the parameters optional ?
So you don't have to to pass it to the method.

src/utils.ts Outdated
if (!param.configKey) {
args[param.parameterIndex] = args[param.parameterIndex];
continue;
if (param.configKey) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, bravoo 👏

@andrew-yustyk
Copy link
Author

andrew-yustyk commented Oct 9, 2018

PR updated.

@shekohex , I've updates parameter types in the tests and Readme as you asked.
Also I've added more strict checking and some comments to applyParamsMetadataDecorator util.
Additional test were added.

Copy link
Member

@shekohex shekohex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well done 😃

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants