fixes #28 and #32 #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
=========================================
+ Coverage 86.97% 87.1% +0.13%
=========================================
Files 62 62
Lines 1090 1101 +11
Branches 190 193 +3
=========================================
+ Hits 948 959 +11
Misses 141 141
Partials 1 1
Continue to review full report at Codecov.
|
| @@ -62,6 +62,7 @@ function computeFinalArgs( | |||
| export function compileFieldResolver( | |||
| target: Function, | |||
| fieldName: string, | |||
| isGetter: boolean, | |||
There was a problem hiding this comment.
Can it be replaced in favor of using Object.getOwnPropertyDescriptor to detect it automatically? Passing it over like this might make it harder to maintain
There was a problem hiding this comment.
it could, but doing Object.getOwnPropertyDescriptor is performance costly. This might sound like a micro optimisation, but it's not. See for example chaijs/chai#127 (comment).
Having it detected only once is worth the faster performance IMHO. Decorators always run statically at startup, so it doesn't make sense to support a scenario where someone would change a property into a getter at runtime.
| const instanceField = | ||
| (source && source[fieldName]) || target.prototype[fieldName]; | ||
|
|
||
| if (isGetter) { |
There was a problem hiding this comment.
This whole block is hard to understand for me. Could we make it external function that would abstract it away with proper name. It seems like the point here is to get target property.
| targetInstance, | ||
| fieldName, | ||
| ); | ||
|
|
||
| fieldsRegistry.set(targetInstance.constructor, fieldName, { |
There was a problem hiding this comment.
If it can be calculated from targetInstance, then passing it as another parameter is redundant in my opinion.
| @@ -15,6 +15,7 @@ export interface FieldInnerConfig { | |||
| property: string; | |||
| description?: string; | |||
| isNullable?: boolean; | |||
| isGetter?: boolean; | |||
There was a problem hiding this comment.
I'm quite strongly against having it explicitly here. It looks like you might define it. But it's part of business logic that could propably be calculated 'on the fly' when needed.
Config is what user or system want it to be. Getter is implementation detail, so I'd say it's not part of config.
There was a problem hiding this comment.
like I've mentioned-detecting getter always using Object.getOwnPropertyDescriptor is quite costly.
I agree this should not be exposed to a client of this library like I did. I do think it's alright to pass down in a private config type. I'll try to refactor.
| if (returnType) { | ||
| return returnType; | ||
| } | ||
|
|
||
| const descriptor = Object.getOwnPropertyDescriptor(target, key); |
There was a problem hiding this comment.
I think this function is already quite complicated and not obvious. Making external function like isGetter would make it more readable and obvious about what is the point here.
| @Field() | ||
| get bar(): number { | ||
| return 42; |
There was a problem hiding this comment.
I don't think it's good way to perform tests by throwing errors to stop them.
There might be different ways to accomplish this like adding instance field and then returning result like return this.foo and that should be enough to test if instance was avaliable
| @@ -14,11 +14,31 @@ describe('Field', () => { | |||
| @ObjectType() | |||
| class Foo { | |||
| @Field() bar: string = 'baz'; | |||
| @Field() undef: boolean = undefined; | |||
There was a problem hiding this comment.
I'd say this test is out of scope of resolving default value. It's rather another test case like Resolves with edge-case default values
|
Thanks for this PR! I've added my code review :) |
|
@pie6k also it doesn't fix the context-in the test you're defining the property on the prototype. We want the instance as the context, not the prototype. |
|
Could you show me code of some failing test for that? |
|
@capaj - I've updated context test to use both prototype and instance context. It's still passing. Let me know if I've missed something you mean :) |
|
@pie6k yeah seems to be working as expected. Good that it doesn't have to do those costly |
No description provided.