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

WIP - Let a user define a custom scalar type by passing a GraphQLScalarType through resolveFunctions #189

Merged
merged 5 commits into from Nov 8, 2016

Conversation

ghost
Copy link

@ghost ghost commented Oct 26, 2016

TODO:

  • Commit messages are formatted in a standard-version friendly way
  • 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

@ghost ghost mentioned this pull request Oct 26, 2016
Copy link
Contributor

@DxCx DxCx left a comment

Choose a reason for hiding this comment

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

@oricordeau Thanks for the contribution, we are almost there.. :)

@@ -68,6 +68,11 @@ function addMockFunctionsToSchema({ schema, mocks = {}, preserveResolvers = fals
defaultMockMap.set('String', () => 'Hello World');
defaultMockMap.set('Boolean', () => Math.random() > 0.5);
defaultMockMap.set('ID', () => uuid.v4());
defaultMockMap.set('JSON', () => ({
Copy link
Contributor

@DxCx DxCx Oct 29, 2016

Choose a reason for hiding this comment

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

i don't think it should be in here,
please define the mock specificly when you use JSON type.
those are the library's default mocks, and JSON is not a default type..

@@ -281,6 +281,14 @@ function addResolveFunctionsToSchema(schema: GraphQLSchema, resolveFunctions: IR
return;
}

if (typeName === 'JSON') {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong as well,
you are not adding the ability to add any custom scalar type, but just the one named JSON.
the patch should just be able to merge any already existing GraphQLObject (Or just Scalar? - @helfer what do you think?) with the schema.

@ghost ghost changed the title Added JSON scalar type WIP - Add JSON scalar type Oct 29, 2016
@ghost
Copy link
Author

ghost commented Oct 29, 2016

Thanks for your feedback ! Now processing it ... ;)

@ghost
Copy link
Author

ghost commented Oct 29, 2016

For those who read, some informations about this PR are available in the corresponding issue :
#176

@stubailo
Copy link
Contributor

Yep, I agree that this PR should be structured allow people to use any GraphQL-JS scalar type, not just JSON.

@ghost
Copy link
Author

ghost commented Oct 31, 2016

Hi, I just made a new commit and now it's supposed to be better ... Tell me.

@ghost ghost changed the title WIP - Add JSON scalar type WIP - Let a user define a custom scalar type by passing a GraphQLScalarType through resolveFunctions Oct 31, 2016
@ghost
Copy link
Author

ghost commented Nov 8, 2016

Hi,

Can someone make a review ? @DxCx ? @stubailo ? I'd like to have an opinion.

Regards,

Olivier

@DxCx
Copy link
Contributor

DxCx commented Nov 8, 2016

@oricordeau hi im on vacation with limited access.
Will take a look in few days if it wont be reviewed by @stubailo in the meanwhile..

@ghost
Copy link
Author

ghost commented Nov 8, 2016

@DxCx Happy vacation, then :)

@stubailo
Copy link
Contributor

stubailo commented Nov 8, 2016

This looks great!

So what should we do with the documentation here: http://dev.apollodata.com/tools/graphql-tools/scalars.html

I guess we should still document some but also document how to use packages like the JSON type?

@stubailo stubailo merged commit 40aa4be into ardatan:master Nov 8, 2016
@DxCx
Copy link
Contributor

DxCx commented Nov 8, 2016

Well done @oricordeau !

@ghost ghost deleted the issue-176 branch November 9, 2016 00:10
@ghost
Copy link
Author

ghost commented Nov 9, 2016

Great !! I'll make a PR on the docs repo soon ...

@ghost ghost mentioned this pull request Nov 14, 2016
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.

None yet

3 participants