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

Adds support for custom enum value resolvers #508

Merged
merged 7 commits into from
Nov 28, 2017

Conversation

orta
Copy link
Contributor

@orta orta commented Nov 27, 2017

We were seeing some issues around enums in our stitched API, artsy/metaphysics#836, and this looked like the most likely candidate.

We built off @clee681's work mainly, wouldn't have known where to start without #364

This might fail because it's waiting on DefinitelyTyped/DefinitelyTyped#21786 - if you'd accept a temporary (thing as any) I can un-block that.

  • If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes) Add support for custom Enum values in schema #363
  • Make sure all of the significant new logic is covered by tests
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Update CHANGELOG.md with your change

/cc @sweir27 @alloy

@apollo-cla
Copy link

@orta: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -32,7 +32,7 @@ export type IResolverObject = {
[key: string]: GraphQLFieldResolver<any, any> | IResolverOptions;
};
export interface IResolvers {
[key: string]: (() => any) | IResolverObject | GraphQLScalarType;
[key: string]: (() => any) | IResolverObject | GraphQLScalarType | any;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps this any is a bit too open? (Looks like it triggers a bunch of additional errors later)

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be GraphQLEnumType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so, that's a full class and really this wants to represent the custom value lookup system which is currently [key: string]: string.

I think I'm gonna go with:

export type IEnumResolver = { [key: string]: string };
export interface IResolvers {
  [key: string]: (() => any) | IResolverObject | GraphQLScalarType | IEnumResolver;
}

Which tightens it, and doesn't create the extra errors in tests

Copy link
Contributor

@stubailo stubailo left a comment

Choose a reason for hiding this comment

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

I'm not really sure how this works, since the instanceof check seems like it would always fail, right?

@@ -32,7 +32,7 @@ export type IResolverObject = {
[key: string]: GraphQLFieldResolver<any, any> | IResolverOptions;
};
export interface IResolvers {
[key: string]: (() => any) | IResolverObject | GraphQLScalarType;
[key: string]: (() => any) | IResolverObject | GraphQLScalarType | any;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could this just be GraphQLEnumType?

@@ -372,6 +373,17 @@ function addResolveFunctionsToSchema(
return;
}

if (type instanceof GraphQLEnumType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wait I'm really confused how this works with resolvers like:

Color: {
  RED: '#EA3232'
},

Since:

{
  RED: '#EA3232'
} instanceof GraphQLEnumType

Is definitely false, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'd agree - but I don't think that's what's being checked for here ( this code is definitely being ran ) when I tried printing out type here it says "Color" which I assume is the description on that class ( I'm still a little new to JS )

@stubailo
Copy link
Contributor

Can we add the (thing as any) temporarily with a TODO comment, and maybe an issue? Curious if the tests pass otherwise.

@stubailo stubailo merged commit 907b8a1 into ardatan:master Nov 28, 2017
@stubailo
Copy link
Contributor

Thank you!

@orta
Copy link
Contributor Author

orta commented Nov 28, 2017

<3 thank you

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.

4 participants