feat: Implement a basic decorator #6
Conversation
virkt25
changed the title
src/decorators/README.md
Outdated
| class MyController { | ||
| @get('/') | ||
| @txIdFromHeader() | ||
| getHandler(txId: string) { |
There was a problem hiding this comment.
It should be a parameter decorator: getHandler(@transactionId() txId: string)
There was a problem hiding this comment.
Pull requests should be focused on a single change, they should not mix new features with tsc setup. I opened #10 to address compilation issues, hopefully that's the last project-setup-related change needed!
The rest of this pull request looks mostly good, please see my few comments below.
Also please remove test/smoke.test.ts, it was a temporary placeholder until we have the first real test.
src/decorators/README.md
Outdated
|
|
||
| ### txIdFromHeader | ||
|
|
||
| This simple decorator allows you to annotate a Controller operation. The decorator will annotate the operation with the value of the header `X-Transaction-Id` from the request. |
There was a problem hiding this comment.
"a Controller operation parameter" or "a Controller method argument"
also
"annotate the operation parameter" or "annotate the method argument"
(use the same variant in both places please)
| import {txIdFromHeader} from '../..'; | ||
| import {Client, createClientForApp} from '@loopback/testlab'; | ||
|
|
||
| describe('@txIdFromHeader() tests', () => { |
There was a problem hiding this comment.
Please move this file to test/acceptance/decorators/txIdFromHeader.acceptance.ts.
- We should use the same directory structure as in
srcfor decorators/controllers/providers etc. - The test file name should use a different type suffix for acceptance and unit tests. In loopback-next core, we are using
.acceptance.ts,.integration.tsand.test.ts, I think we should do the same here. Maybe use.unit.tsinstead of.test.ts?
For the top-level describe block, our convention in loopback-next is to add "(acceptance)" or "(unit)" to the name, e.g. describe('@tsIdFromHeader() (acceptance)').
test/unit/txIdFromHeader.test.ts
Outdated
| import {get, getControllerSpec} from '@loopback/core'; | ||
| import {txIdFromHeader} from '../..'; | ||
|
|
||
| describe('@txHeaderFromId', () => { |
There was a problem hiding this comment.
Please move this file to test/unit/decorators/txIdFromHeader.test.ts. Considering that this is a unit-test that should be executing only a single class (single source file), it may be better to include ".decorator" type in the file name too, e.g. test/unit/decorators/txIdFromHeader.decorator.test.ts.
There was a problem hiding this comment.
There are still two files that have the same filename txIdFromHeader.test.ts and differs by the path (acceptance vs. unit). I makes it more difficult for me to quickly understand what file I am reading or editing, because the path is not always shown or is de-emphasized in the UI.
Other than that, the patch LGTM. No further review is needed as far as I am concerned.
I think you may have took the idea that project infrastructure changes should go to standalone pull requests too far. New dependencies are usually added as part of the pull request that is adding code using them. Not a big deal, I'm fine to land #11 first and then this #6 next. |
virkt25
force-pushed
the
decorator
branch
3 times, most recently
from
b688c4a to
b625ad6
Compare
Implement a `@txIdFromHeader()` decorator that can annotate a controller operation to receive the value of `X-Trasaction-Id` Header.
Implement a
@txIdFromHeader()decorator that can annotate acontroller operation to receive the value of
X-Trasaction-IdHeader.connect to loopbackio/loopback-next#525