Fix preserveResolvers for root resolvers

[sebastienbarre]

Here is a test that show that that last commit in #96 broke preserveResolvers for root resolvers.
Turns out, that test was already there, but no mock was in mockMap so nothing could take precedence.

@helfer I'll work on a fix.

[sebastienbarre]

And here is the fix, @helfer

[sebastienbarre]

I spoke too fast. None of this code, or the previous fix for that matter, take into account the fact that mocks and resolvers can return promises. Will fix.

[sebastienbarre]

Alright, there was more than a few problems with the implementations from 3 weeks ago (besides the issue with root resolver fixed in the previous commit). We were trying to merge both mock and resolver.

      field.resolve = (...args) => {
        const mockedValue = mockResolver(...args);
        const resolvedValue = oldResolver(...args);
        return typeof mockedValue === 'object' && typeof resolvedValue === 'object'
          ? Object.assign({}, mockedValue, resolvedValue) : resolvedValue;
      };

1. this wouldn't work if either mockedValue or resolvedValue were promises, which are supported by GraphQL (I added a test),
2. typeof mockedValue === 'object' was just wrong, since Javascript arrays are also objects (I added a test),
3. Object.assign() only works for enumerable properties. This is a problem if either mockedValue or resolvedValue use Object.defineProperty to implement access to their properties (I added a test). This is a real issue if you use the popular sequelize ORM for example. Say, you store records in a database, with the name column. If you retrieve a row using sequelize's findOne, you will get an object foo, which does not have an enumerable name property, but defines one using Object.defineProperty. It works perfectly fine when fed to GraphQL (via graphql-sequelize for example), because the GraphQL execution engine will access the property using foo.name or foo['name'] down the road. There is no assumption as to whether that property is enumerable or not (and owned).

Here is the new version (see commit in that PR):

      field.resolve = (...args) => Promise.all([
        mockResolver(...args),
        oldResolver(...args),
      ]).then(values => {
        const [mockedValue, resolvedValue] = values;
        if (isObject(mockedValue) && isObject(resolvedValue)) {
          return new Proxy({}, {
            get: (target, key) => {
              const value = resolvedValue[key];
              return value === undefined ? mockedValue[key] : value;
            },
          });
        }
        return resolvedValue;
      });

1. uses Promise.all() to handle promises,
2. uses isObject() to ignore arrays,
3. returns a Proxy that overrides the dot and bracket operators and uses the corresponding key in resolvedValue or in mockedValue otherwise.

Now the issue with Proxies is that they can not be transpiled by Babel. They are supported by Node 6. I don't know what kind of Node compatibility you guys want to maintain.

You could return a plain object instead of the Proxy:

       return Object.assign({}, mockedValue, resolvedValue);

but you would give up support for properties defined through Object.defineProperty.

What do you think @helfer?

UPDATE: these is a krzkaczor/babel-plugin-proxy plugin, but it seems to have a performance impact. Maybe speed is not as important in the context of mocking?

[helfer]

Oh, wow, I clearly didn't look closely enough then. The dangers of relying too much on tests when they don't actually cover enough possible cases...

At the moment I think we cannot support only node 6, we also have to support node 4. I think it's fine to not support Object.assignProperty properties, we just have to document it somewhere.

Thanks a lot, by the way, this is great stuff! Once you rebase the PR, I will take a careful look this time.

[sebastienbarre]

@helfer OK done (hopefully). I reverted back to Object.assign() instead of Proxy. I left the Proxy solution and the corresponding test in comments, if you don't mind.

[helfer]

Can you add a longer comment here that references the code that it would be testing (which is also commented out)

[helfer]

Ok, perfect! I merged another PR, so you need to do another rebase.If you have time, it would be great if you could add that comment I mentioned in the test as well. Ping me on slack when you've rebased, so I can merge it right away so we don't have to do another rebase etc.

PS: Oh, and if you could add an entry in the changelog that would be very nice as well.

[helfer]

Looks like the linter is not happy. Maybe you can try defining the function that runs inside the forEach outside the while loop? Or even better outside the sources.forEach.

[helfer]

Awesome, thanks so much!

[cloudflare-pages]

Deploying with    Cloudflare Pages

Latest commit:	61cf64b
Status:	🚫  Build failed.

View logs