Skip to content

Conversation

IvanGoncharov
Copy link
Member

No description provided.

@IvanGoncharov IvanGoncharov added PR: breaking change 💥 implementation requires increase of "major" version number and removed CLA Signed labels Jul 3, 2018
@IvanGoncharov IvanGoncharov added this to the v14.0.0 milestone Jul 3, 2018
extensionASTNodes,
serialize: type._scalarConfig.serialize,
parseValue: type._scalarConfig.parseValue,
parseLiteral: type._scalarConfig.parseLiteral,
Copy link
Member Author

@IvanGoncharov IvanGoncharov Jul 3, 2018

Choose a reason for hiding this comment

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

At the moment to copy scalar config you should either use type._scalarConfig.serialize or type.serialize.bind(type).
This is a big problem for 3rd-party libraries doing schema transformations.

@mjmahone
Copy link
Contributor

@IvanGoncharov sorry I feel like I'm missing context on this: it seems like an improvement based purely on the fact that the flow types are better, but other than that I am not sure I see why this is desired? The change is straightforward enough I may merge regardless, but wanted to understand your logic.

@IvanGoncharov
Copy link
Member Author

@mjmahone For example, I want to add a prefix to every type in a schema:

function prefixType(type: GraphQLNamedType, prefix: string): GraphQLNamedType {
  if (isScalarType(type)) {
    return new GraphQLScalarType({
      name: prefix + type.name,
      // ...
      parseLiteral: type.parseLiter,
    });
  }
  // ...
}

And parseLiteral: type.parseLiteral is a bug because parseValue, parseLiteral and serialize are method and not a callbacks. And it's very different from all other type-related callbacks like resolve, subscribe, isTypeOf, etc.

Moreover you either need to use private property _scalarConfig or to create wrapper function using bind that would be called every time we graphql-js parses literal. Also if we use bind that mean newly created scalar has refence to old scalar so they can be garbage collected only together.


// Serializes an internal value to include in a response.
serialize(value: mixed): mixed {
const serializer = this._scalarConfig.serialize;
Copy link
Member Author

@IvanGoncharov IvanGoncharov Jul 16, 2018

Choose a reason for hiding this comment

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

it seems like an improvement based purely on the fact that the flow types are better

@mjmahone This wrapper function depends on this so before proposed change you can't do:

const serialize = GraphQLString.serialize;
serialize(5);
// TypeError: Cannot read property '_scalarConfig' of undefined

Or more complicated scenario:

const newScalar = new GraphQLScalarType({
  name: GraphQLString.name,
  serialize: GraphQLString.serialize,
});
console.log(GraphQLString.serialize(5))
// 5
console.log(newScalar.serialize(5))
// TypeError: Cannot read property '_scalarConfig' of undefined

@mjmahone
Copy link
Contributor

Great, thanks for the context @IvanGoncharov !

@mjmahone mjmahone merged commit 830907b into graphql:master Jul 16, 2018
@IvanGoncharov IvanGoncharov deleted the scalarCallbacks branch July 18, 2018 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: breaking change 💥 implementation requires increase of "major" version number

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants