Skip to content

Feature - Union Type#21

Merged
felipesabino merged 4 commits intomasterfrom
feature/union
Oct 25, 2017
Merged

Feature - Union Type#21
felipesabino merged 4 commits intomasterfrom
feature/union

Conversation

@felipesabino
Copy link
Copy Markdown

@felipesabino felipesabino commented Oct 25, 2017

  • Adding Union Type decorator
  • Started using a more modular approach to decorators and metadata (see Refactor decorators class structure #22)
    • Avoided usage of Reflect in favor to a well defined interface (metadata storage) for metadatas (inspired by [class-validator(https://github.com/pleerock/class-validator)] and routing-controllers style)
    • The goal is to slowly refactor other decorators, factories and arg options to the new structure to make code maintenance easier

Comment thread src/schema_factory.spec.ts Outdated
import 'reflect-metadata';

import * as D from './decorator';
import * as DD from './decorator/';
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I fell a little bit uneasy with these "duplicated" decorators

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

managed to expose everything from inside the old decorator.ts file, guess it is better like that while all the other decorators are in the old structure.

Comment thread src/metadata/options/union.option.ts Outdated
/**
* Arguments for a Union type on graphql schema
*/
export interface UnionOpton<T> {
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

typo

Comment thread src/type-factory/union.type-factory.ts Outdated
.filter(_ => _), //filter null values
});
})
.find((_, index) => index === 0) || null;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is the purpose of this || null?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

just to be explicit in the return type instead of returning undefined. I actually could have set the function return type to be graphql.GraphQLUnionType | undefined instead 👍

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