fix for custom scalar client-side usage

[Urigo]

@glasser

Fixes #225

[DxCx]

is there someone trying to fix this?
or i'll give it a go?

[glasser]

Can't hurt! Doubt anyone will look at it until California Monday at the earliest anyway...

…

On Dec 11, 2016 12:41 AM, "Hagai Cohen" ***@***.***> wrote: is there someone trying to fix this? or i'll give it a go? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#224 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AABBVFM203VPeHLvn6zwF0RL0UbOLVmyks5rG7cugaJpZM4LHypv> .

[DxCx]

Ok,
so i did look into it, and found the bug.
basiclly, to create a scheme, first we will use graphql to generate typedef to scheme.
when doing so, a GraphQLScalarType is being created, with default parse functions.

later on, we will run the function to bind resolvers.
However, this specific line: https://github.com/apollostack/graphql-tools/blob/master/src/schemaGenerator.ts#L280
replaces the placeholder scalar type with the one provided.
and then overrides the original type with the fields from the given scalar.
https://github.com/apollostack/graphql-tools/blob/master/src/schemaGenerator.ts#L293

however, all the arguments of the fields (which has arguments ofcourse) did not replace type.
so when comparing between the two (argType === scalarType) we will get a missmatch.

so, to sum this up, we have couple of choices:

1. run a "fix" function on the scheme to fix arguments after resolvers placed.

• IMO, this approach will hit performance of scheme compiling.

2. remove this chunk of code which replaces the type on map:

    if (type instanceof GraphQLScalarType) {
      const resolveFn = resolveFunctions[typeName];
      if (resolveFn instanceof GraphQLScalarType) {
        const thisScalarType: GraphQLScalarType = resolveFn;
        schema.getTypeMap()[typeName] = thisScalarType;
      }
    }

• however, this means that the original scalar given is not really used, only a copy of the handling functions. (easier fix)

@Urigo / @glasser / @helfer what do you think?

[stubailo]

@DxCx I'd say we should just replace the functions and not the actual scalar instance.

[stubailo]

(Like your approach 2)

[glasser]

So literally just delete the quoted block? That does appear to make the whole test suite pass (well, Uri's test actually has to be adjusted slightly to expect(result.errors).to.not.be.ok; instead of expect(result.errors.length

[DxCx]

ok @Urigo,
i just pushed the fix on your branch and updated the PR.
@helfer / @stubailo please approve and merge :)