use function reference instead of string for concatenateTypeDefs

[nerdmed]

hey @stubailo - i spend some hours traveling through my code and searching for an issue we found in our project after we started to export our typeDefs inside of objects that get combined with a function. The problem seems to be with the way the tools are checking how to include a

Our objects are structured like this:

import TypeB from './TypeB';

const TypeA = {
    typeDefs: `
        type TypeA {
            name: String!
            field: TypeB
        }
        `,
    resolvers: {
        TypeA: {
            name: () => null,
            field: () => null,
        },
    },
};

export default combineSchemaDependencies(TypeA, TypeB);

typeDefs can be a function returning an array or a string like you expect for the typeDefinitions passed into makeExecutableSchema. As soon as i export my objects via my function there are no typeDefinitions in the result. The reason for that is that i combine the types like this:

function combineSchemaDependencies(...args) {
    return {
        typeDefs: () => args.map(a => a.typeDefs),
        resolvers: args.reduce((result, obj) => Object.assign(result, obj.resolvers), {}),
    };
}

What i debugged is that the concatenateTypeDefs function decides if a function that is inside the array will be resolved or not. It stores the functions that have been called as an key inside an object. That causes the function to become a string. Something like that () => [TypeA, TypeB] - In my case we have a function generating the function that will return the function that returns the array 💃 . That causes that the string that will be saved into the array is () => args.map(a => a.typeDefs) - and then its the same for every export with the combine function and none of my types gets imported into the schema.

So to allow circular dependencies while not having to rely on a string of the function that is checked i would suggest to simply save the reference of the function into an array and then decide depending on the reference. This works for circular dependencies as the imports are only done once and therefore the reference should stay the same. I have tried to create a test for my case but TypeScript drove me crazy as i am not so familiar with it.

Let me know if you have some thoughts on this or any feedback.

[apollo-cla]

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

[nerdmed]

Tests are passing for Node 6 but not for 4.4 cause i am using Array.includes - should be fine after we change it to Array.indexOf()

Update: changed the code to use indexOf instead of includes.

[ephemer]

Nitpick: Indentation

[nerdmed]

thx - fixed

[helfer]

@nerdmed good catch! I'm a bit worried about the performance implications of using an array, but I think as long as the schema isn't huge and in hundreds of files it shouldn't matter much. What do you think @DxCx?

[ephemer]

@helfer I had the same reservation when I saw it but

1. It's only a one-off on startup
2. Duplicates don't make it into the array at all so it's not as bad as it could be. It seems incredibly unlikely that it'll ever have more than 1000 elements even in the most extreme imaginable case - and at that point I doubt this implementation would be the sticking point

In the worst case it'd be easy enough to optimise this into a binary tree of some kind, which would return it to the same performance characteristics of the hashed object. I'd suggest it's not worth the extra code complexity though

[nerdmed]

rebased to avoid merge-commit

[DxCx]

you are missing tab in here

[nerdmed]

fixed & thx

[DxCx]

hi,
@helfer, performance wise, assuming the array doesn't have holes (which is the case right now, since he is only pushing) the performance for array will be better.

now, @nerdmed, logic-wise, i don't see what's the difference in storing the the ref inside an object keymap or in array, can you explain how this change solved your bug?
also, please add tests to your case, we will have to make sure behavior doesn't change in the future. if you need help with typescript errors please let me know and i'll help, you can find me on slack (@HagaiCo)

[nerdmed]

@DxCx i agree that a test is needed to prevent the issue in the future. The issue comes from the following behaviour in javascript:

function add10(number){
	return number + 10;
}

const myObject = {};
myObject[add10] = 1;
console.log(myObject);
// -> Object {"function add10(number){↵	return number + 10;↵}": 1}

This can have strange side effects as we are comparing the string of a function for the next iteration. I can imagine a couple of side effects from using a string comparison to decide if a function has been invoked. As soon as the function is in a closure your function string can be the same but the results can be completely different. In case of the the reference you can at least catch the cases where you have something like a factory creating the functions you are going to compare. Every functions works same, has the same string, but has a different reference hence will not fail in this check. To make this 100% pullet proof one should compare the result of the function because this is what we care about. But this gets tricky with circular deps.

I will try to create a case for that without going into much detail from our project. But does this make the reason more clear?

[DxCx]

ok, yeah, i get it, forgot that keys converted into string ;)
so yeah, you are absolutely right, no place for object-keys in here.
and this is a really nice catch IMO 👍

so now only the tests are missing ;)

[nerdmed]

@DxCx alright found some time to create a test case. This is the simplest i got out of my mind. I hope it is understandable. Maybe you have a better idea for the test description? The new test case will fail on the current master for TypeC and TypeD as they will never pass the concatenateTypeDefs function as all following typeDefs created by the function will get compared to the string () => args.map(o => o.typeDefs)!

[nerdmed]

hey @DxCx, any feedback on this?

[DxCx]

LGTM :+1

[DxCx]

Sorry i missed it :)
Good thing you pinged again..
@helfer can we merge this?

[helfer]

Thanks for the reminder!

[nerdmed]

@helfer thanks for merging!