bugifx:mock interface type

[bchenSyd]

it didn't work for me by following

http://dev.apollodata.com/tools/graphql-tools/mocking.html#Mocking-interfaces

so I fixed the problem

[apollo-cla]

@bochen2014: 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/

[bchenSyd]

hi @helfer @DxCx , can you please merge my PR as we need this in our project?
thanks

[bchenSyd]

is this project still maintained? @helfer

[stubailo]

@bochen2014 yes definitely - sorry, we've been traveling to some conferences recently and it's currently a Sunday. Let me take a look now.

[stubailo]

Hmm, I just looked around and it's my mistake that this made it into the docs.

Looks like it started with this issue: #171
Then this PR, which I merged incorrectly even though it didn't work: https://github.com/apollographql/tools-docs/pull/53

The original docs content submitted in the PR had a resolver that looked at typename on the object to determine the type, but then that was removed even though it definitely doesn't work without that.

[stubailo]

I've temporarily reverted that docs change: https://github.com/apollographql/tools-docs/commit/d243bf0da9743c3a9e2320b735f18dd06c852a71

I think the right implementation here should have the following properties:

1. Same behavior for interfaces and unions
2. Use a random member of the interface/union as a default, as before
3. If there is a __typename field on the object, uses that

I think using __typename instead of typename makes sense since it's closer to how GraphQL returns that information.

[stubailo]

Let me know if the above suggestions seem good to you.

[bchenSyd]

thanks @stubailo . yes your comments makes sense to me.
I've updated the PR with your suggestions (mostly typename->__typename)

all unit tests passed (including 2 new tests which I added as part of this PR). can you please review again? thanks

[bchenSyd]

add explanation

[bchenSyd]

the whole idea of this PR is that, when you run node(id:'bee:123') query, the mock server won't randomly choose a Flying type (could be either Bee or Bird ) but will run your mock of Flying code (where you specify whether to return a bee or bird ). It basically enables user to implement a id-fetcher for their node interface (which is where this PR comes from)

[stubailo]

Yes, but couldn't we support both things? Like if your mock returns a __typename that is used (which supports your case) but if you don't want to care about that it will select a random one?

[bchenSyd]

correct. thats the current behavior.. still the same

[stubailo]

Can we make sure that Unions behave the same way? https://github.com/apollographql/graphql-tools/pull/332/files#diff-483b6e7e62e3109d7c87ae3c54425430R199

I think that means factoring out the type selection logic into a function.

[bchenSyd]

will do it this morning... thanks

[bchenSyd]

hi @stubailo I've updated my PR to address unionTypes explicit mock. please review it again..

plus, your updated doc

http://dev.apollodata.com/tools/graphql-tools/mocking.html#Mocking-interfaces

seems to be a bit misleading.. I think for most users, leaving the parameter preserveResolvers in addMockFunctionsToSchema to default false would be a much better choice because

1. it will trigger the random selection of possible implementation types for that interface/union and a typename (now renamed to __typename) will provided by the framework (so user don't have to care about it).
2. it also will tell the framework to call assignResolveType to inject a default resolveType function, which will look up the __typename property of the resolved result

[stubailo]

@bochen2014 that's a good point.

I'm going to review and merge this today, do you mind if I make some code changes along the way?

[bchenSyd]

feel free to change whatever you think is required ;-)

[bchenSyd]

any updates?😏

[bchenSyd]

hi @stubailo have you got a chance to have a review? we actually need the change in our project . thanks

[helfer]

Thanks @bochen2014 for the great PR and sorry for the delay! I will talk with @stubailo today and we'll get the PR merged asap! 🙂

[helfer]

Ok, I think this change is pretty good, but it's also a breaking change (because we're changing typename to `__typename)., so if we merge it we'll have to release version 2.0

edit: since we never documented the automatic behavior with __typename, it should be ok to release it as a minor version.

[bchenSyd]

thanks @helfer for merging my pr!