Pull request for issue #521 https://github.com/inversify/InversifyJS/… #41

dcavanagh · 2017-03-31T21:53:20Z

Add Decorators for binding method parameters to request parameters like in Spring or node-decorators

Description

@request - binds a method parameter to the request object - still provided at the end of the arguments list for backwards compatibility
@response - binds a method parameter to the response object - still provided at the end of the arguments list for backwards compatibility
@RequestParam - binds a method parameter to request.params or to a specific parameter if a name is passed
@QueryParam - binds a method parameter to request.query or to a specific query parameter if a name is passed
@RequestBody - binds a method parameter to request.body or to a specific body property if a name is passed
@RequestHeaders - binds a method parameter to the request headers
@Cookies - binds a method parameter to the request cookies
@Next - binds a method parameter to the next() function - still provided at the end of the arguments list for backwards compatibility

Related Issue

issue 521
inversify/InversifyJS#521

Motivation and Context

Better developer experience than using request and response
modeled after spring and node-decorators
some code reused from node-decorators

How Has This Been Tested?

unit tests for new code
original unit tests still pass
backwards compatible
node v7.4.0

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

Dirrk

I just wanted to point out some minor things I noticed that probably weren't meant to be in the PR that can save someone some time before they actually review this.

Dirrk · 2017-03-31T23:59:59Z

README.md

+
+### `@RequestBody(name?: string)`
+    binds a method parameter to request.body or to a specific body property if a name is passed-']'ok89im.jo;iuh:'
+


Did you mean to commit this?

']'ok89im.jo;iuh:'

Dirrk · 2017-04-01T00:01:26Z

.vscode/settings.json

-        "**/dist": true,
-        "**/docs": true,
-		"type_definitions/**/*.js": true
+ 		"**/.DS_Store": true,


This looks like auto formatting was it intended to be committed?

Dirrk · 2017-04-01T00:03:42Z

src/decorators.ts


 export function Controller(path: string, ...middleware: interfaces.Middleware[]) {
    return function (target: any) {
-        let metadata: interfaces.ControllerMetadata = {path, middleware, target};
+        let metadata: interfaces.ControllerMetadata = { path, middleware, target };


Again I think this is your ide making an unexpected change?

Dirrk · 2017-04-01T00:04:05Z

src/decorators.ts

@@ -39,7 +39,7 @@ export function Delete(path: string, ...middleware: interfaces.Middleware[]): in

 export function Method(method: string, path: string, ...middleware: interfaces.Middleware[]): interfaces.HandlerDecorator {
    return function (target: any, key: string, value: any) {
-        let metadata: interfaces.ControllerMethodMetadata = {path, middleware, method, target, key};
+        let metadata: interfaces.ControllerMethodMetadata = { path, middleware, method, target, key };


Dirrk · 2017-04-01T00:10:43Z

src/constants.ts

 };

+export enum ParameterType {


Should this be capitalized like the other constants?

lholznagel · 2017-04-01T00:37:46Z

Thanks!
Will also take a look later

dcavanagh · 2017-04-01T12:08:12Z

@Dirrk Thanks for pointing those things out. I noticed some other things that were auto formatted as well. I was rushing because I had everything set up to submit it and my kids got home and interrupted me. Should I resubmit the pull request?

lholznagel · 2017-04-01T12:25:05Z

@AltekkeE just push more commits to the current branch. It will auto update this pr :)

This reverts commit ee1b51c.

Changed ParameterType to PARAMETER_TYPE Formatting in README.md

dcavanagh · 2017-04-01T12:48:46Z

@lholznagel - Done.

I also notice that the code coverage report is not working correctly. It says 100% but no files are actually checked. This may be happening in the main inversify repo too.
-edit: removed copy of previous comment from email

lholznagel · 2017-04-01T12:57:29Z

README.md

@@ -42,12 +42,17 @@ export class FooController implements interfaces.Controller {
    constructor( @inject('FooService') private fooService: FooService ) {}

    @Get('/')
-    private index(req: express.Request): string {
+    private index(req: express.Request, res: express.Response, next: express.NextFunction): string {


Couldn´t we just use @QueryParam('id') id: string?

I wanted to show that you can still do it the original way

Oh okay.
Makes sense

lholznagel · 2017-04-01T14:06:21Z

LGTM 👍

remojansen · 2017-04-02T13:49:24Z

👍 Amazing PR 💯 thanks a lot for this contribution. One question: did you find a way to fix the test coverage reports?

lholznagel · 2017-04-02T13:57:53Z

@remojansen yesterday I checked travis and the problem is also in the main repo since some time.
I did a quick look to find a solution, but until now I didn´t find anything.

remojansen · 2017-04-02T14:23:54Z

I think is probably some breaking change in one of the tools but is hard to find out which one :(

dcavanagh · 2017-04-02T14:44:19Z

I looked at it briefly but I didn't know if you were doing it for transpired code or for typescript. It could have to do with not having source maps enabled. I had found some other people that ran into a similar problem but forget if it was recent. They solved it by making a new gulp plugin for typescript to Istanbul

…

On Sun, Apr 2, 2017 at 10:23 AM Remo H. Jansen ***@***.***> wrote: I think is probably some breaking change in one of the tools but is hard to find out which one :( — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#41 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKvP2NmKP9fPYCSEyWmJhfLovbrCP2N7ks5rr697gaJpZM4MwMrL> .

remojansen · 2017-04-02T15:03:46Z

Thanks a lot I will create an issue to track this down and see if we can fix it 👍

remojansen · 2017-04-02T18:51:55Z

Just release [email protected] which includes this feature 🎉

remojansen · 2017-04-03T10:33:27Z

Hi @AltekkeE I have noticed that in the docs you mention @RequestParams but it seems to be not available in the source code. Did you forget to commit it? Thanks!

dcavanagh · 2017-04-03T13:14:56Z

@remojansen I will take a look right now

dcavanagh · 2017-04-03T13:19:52Z

😬 Spelling mistake, Should be @RequestParam in the docs. It is correct down in the description part just not in that delete statement

remojansen · 2017-04-03T13:22:20Z

No worries.Would u like to send a PR?

dcavanagh · 2017-04-03T15:05:16Z

@remojansen [Gulp Problem] The problem is with gulp-mocha v4. I downgraded to ^3.0 and it works again.

In a project that I am working on we are not using gulp. We just run the nyc using npm

"scripts": {
    "start": "node ./dist/bootstrap.js",
    "postinstall": "tsc",
    "test": "mocha --opts mocha.opts && rm -rf test",
    "test:dev": "mocha --opts mocha.opts --watch",
    "cover": "nyc npm run test"
  }

the issue below points to this as well.

SBoudrias/gulp-istanbul#115

@RequestParam

* Fixed README.md @RequestParams to @RequestParam * After fixing the code coverage output I noticed that some lines were not covered Fixed @Cookies framework test to cover code in server.ts Removed unused code from decorators.ts

dcavanagh and others added 2 commits March 31, 2017 17:48

Pull request for issue #521 inversify/InversifyJS#521

31f9406

Reset settings.json

ee1b51c

Dirrk reviewed Apr 1, 2017

View reviewed changes

dcavanagh added 2 commits April 1, 2017 08:34

Revert "Reset settings.json"

b925649

This reverts commit ee1b51c.

Reverted unintentional formatting changes.

05a1e36

Changed ParameterType to PARAMETER_TYPE Formatting in README.md

lholznagel reviewed Apr 1, 2017

View reviewed changes

lholznagel approved these changes Apr 1, 2017

View reviewed changes

remojansen merged commit b994a59 into inversify:master Apr 2, 2017

remojansen mentioned this pull request Apr 2, 2017

Fix test coverage reports inversify/InversifyJS#523

Closed

dcavanagh mentioned this pull request Apr 3, 2017

Minor Fixes for Pull Request #41 #42

Merged

9 tasks

dcavanagh mentioned this pull request Apr 4, 2017

Add Decorators to index.ts for #41 #43

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pull request for issue #521 https://github.com/inversify/InversifyJS/… #41

Pull request for issue #521 https://github.com/inversify/InversifyJS/… #41

dcavanagh commented Mar 31, 2017 •

edited

Loading

Dirrk left a comment

Dirrk Mar 31, 2017

Dirrk Apr 1, 2017

Dirrk Apr 1, 2017

Dirrk Apr 1, 2017

Dirrk Apr 1, 2017

lholznagel commented Apr 1, 2017

dcavanagh commented Apr 1, 2017 via email •

edited

Loading

lholznagel commented Apr 1, 2017

dcavanagh commented Apr 1, 2017 •

edited

Loading

lholznagel Apr 1, 2017

dcavanagh Apr 1, 2017

dcavanagh Apr 1, 2017

lholznagel Apr 1, 2017

lholznagel commented Apr 1, 2017

remojansen commented Apr 2, 2017

lholznagel commented Apr 2, 2017

remojansen commented Apr 2, 2017

dcavanagh commented Apr 2, 2017 via email

remojansen commented Apr 2, 2017

remojansen commented Apr 2, 2017

remojansen commented Apr 3, 2017

dcavanagh commented Apr 3, 2017

dcavanagh commented Apr 3, 2017

remojansen commented Apr 3, 2017

dcavanagh commented Apr 3, 2017 •

edited

Loading


		### `@RequestBody(name?: string)`
		binds a method parameter to request.body or to a specific body property if a name is passed-']'ok89im.jo;iuh:'

Pull request for issue #521 https://github.com/inversify/InversifyJS/… #41

Pull request for issue #521 https://github.com/inversify/InversifyJS/… #41

Conversation

dcavanagh commented Mar 31, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Dirrk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lholznagel commented Apr 1, 2017

dcavanagh commented Apr 1, 2017 via email • edited Loading

lholznagel commented Apr 1, 2017

dcavanagh commented Apr 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lholznagel commented Apr 1, 2017

remojansen commented Apr 2, 2017

lholznagel commented Apr 2, 2017

remojansen commented Apr 2, 2017

dcavanagh commented Apr 2, 2017 via email

remojansen commented Apr 2, 2017

remojansen commented Apr 2, 2017

remojansen commented Apr 3, 2017

dcavanagh commented Apr 3, 2017

dcavanagh commented Apr 3, 2017

remojansen commented Apr 3, 2017

dcavanagh commented Apr 3, 2017 • edited Loading

dcavanagh commented Mar 31, 2017 •

edited

Loading

dcavanagh commented Apr 1, 2017 via email •

edited

Loading

dcavanagh commented Apr 1, 2017 •

edited

Loading

dcavanagh commented Apr 3, 2017 •

edited

Loading