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

Feature - Refactoring @Field decorator #34

Merged
merged 9 commits into from
Nov 7, 2017

Conversation

felipesabino
Copy link

@felipesabino felipesabino commented Oct 29, 2017

Sorry but this will be a big PR with a lot of breaking changes


  • Refactored @Field decorator and nested dependencies (@Arg, @OrderBy, @Root, @Before and @Ctx) - Refactor decorators class structure #22
  • Renamed DefaultOption, DefaultMetadata and DefaultArg interfaces to just Option, Metadata and Argument instead as in Feature - Refactoring more decorators #32 (comment)
  • Refactored metadata handling (to use MetadataBuilder instead of Reflect)
  • Refactored field_type_factory to handle metadata properly and to be less verbose/confusing
  • Added functional tests (to make sure resolvers are working properly)
  • Removed old (and commented) code
  • Updated examples
  • Review JSDoc for each decorator and its option argument - Improve documentation #23
  • Review TODOs for possible missing tests
  • Deprecated all usages of
    • @Pagination
    • @Description
    • @List
    • @NonNull

@felipesabino felipesabino changed the title [WIP] Featue - Refactoring @Field decorator Featue - Refactoring @Field decorator Nov 4, 2017
@felipesabino felipesabino changed the title Featue - Refactoring @Field decorator Feature - Refactoring @Field decorator Nov 4, 2017
Copy link

@tibawatanabe tibawatanabe left a comment

Choose a reason for hiding this comment

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

2 comments:

  • I think there are some unneeded imports of reflect-metadata
  • ArgumentArg... a little bit strange but I have no better suggestion

@@ -0,0 +1,27 @@
import 'reflect-metadata';

Choose a reason for hiding this comment

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

Do we need this here? I'm not finding a need...

Copy link
Author

Choose a reason for hiding this comment

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

I think there are some unneeded imports of reflect-metadata

I was getting a lot of error like

Uncaught TypeError: Reflect.getMetadata is not a function

So this helped me suppress it... found this solution here but do not know a better alternative.

Do you know the root cause for it? Any idea if something at tsconfig might fix this issue without these imports?

ArgumentArg... a little bit strange but I have no better suggestion

Same here 😞

Copy link
Author

Choose a reason for hiding this comment

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

@tibawatanabe

I think there are some unneeded imports of reflect-metadata

Ok, I did some reading and I need it just in my app/lib entry point, so I added it to both index.ts and *.spec.ts

@felipesabino felipesabino merged commit 20a0a3c into master Nov 7, 2017
@felipesabino felipesabino deleted the feature/refactor-field branch November 7, 2017 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants