prismake / typegql

Create GraphQL schema with TypeScript classes.
https://prismake.github.io/typegql/
MIT License
423 stars 21 forks source link

fixes #28 and #32 #35

Closed capaj closed 6 years ago

codecov[bot] commented 6 years ago

Codecov Report

Merging #35 into master will increase coverage by 0.13%. The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/domains/field/registry.ts 100% <ø> (ø) :arrow_up:
src/services/utils/gql/types/parseNative.ts 92% <100%> (+1.09%) :arrow_up:
src/domains/field/compiler/index.ts 90.62% <100%> (ø) :arrow_up:
src/domains/field/index.ts 79.16% <100%> (+0.9%) :arrow_up:
src/domains/field/compiler/resolver.ts 91.76% <100%> (+0.73%) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update b1f26e6...33cb7ba. Read the comment docs.

pie6k commented 6 years ago

Thanks for this PR! I've added my code review :)

pie6k commented 6 years ago

@capaj I've played a bit with it and actually it could be solved in much simpler way: I've created PR: https://github.com/prismake/typegql/pull/37- it seems to solve both #28 and #32 (check added test cases) too.

capaj commented 6 years ago

@pie6k this only fixes the #32, not the #28 or am I missing something?

capaj commented 6 years ago

@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.

pie6k commented 6 years ago

Could you show me code of some failing test for that?

pie6k commented 6 years ago

About #28 - https://github.com/prismake/typegql/pull/38 fixes that. Again it's way simpler than here because it doesnt include getters workaround there

pie6k commented 6 years ago

@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 :)

capaj commented 6 years ago

@pie6k yeah seems to be working as expected. Good that it doesn't have to do those costly Object.getOwnPropertyDescriptor calls. :+1: