add mergeAST from GraphiQL, refactor using visit() [WIP]

[acao]

Accomplishes graphql/graphiql#836, originally introduced via graphql/graphiql#762 earlier this year, mergeAST is a handy utility for in-lining fragments for complex queries.

As per @IvanGoncharov's suggestion, I refactored it to use language/visitor's visit function. Good thing I'd been working with AST and visitors on another project using antlr4 earlier this year, or I would've been so lost here, haha. That said, GraphQL's visit() is so powerful, I'm blown away at how simple this was.

Thank you @ramonsaboya for the inspiration, and some solid unit tests, and an excellent idea all around.

I'm wondering if a Set would be a more appropriate data structure here?

This is a WIP, just learning about visit() and this is just a learning excercise for me.
Not a huge priority to any part of the ecosystem currently, and a lot of important edge cases are still broken here.

[acao]

I found a failing case that wasn't in the original unit tests:

      query Test1 {
        ...Fragment4
        ...Fragment5
      }
  
      fragment Fragment5 on Test1 {
        ...Fragment4
      }

      fragment Fragment4 on Test1 {
        id
      }

does not give me

 query Test1 {
        ...on Test1 {
          id
        }
        ...on Test1 {
          ...on Test1 {
            id
          }
        }
      }

instead, I get:

 query Test1 {
        ...on Test1 {
          id
        }
        ...on Test1 {
          id
        }
      }

this works in the original! hmmm 🤔

[IvanGoncharov]

You don't need visit here, simple for of over definitions would be enough.

[IvanGoncharov]

We don't use fixtures please unroll it here.
Use dedent to remove indentation from strings.
And defined expectMerged wrapper to not repeat print(mergeAST(parse

[IvanGoncharov]

Please write it like this:

function expectInlined(query) {
  return expect(print(inlineNamedFragments(parse(query))));
}

describe('inlineNamedFragments', () => {
  it('does not modify query with no fragments', () => {
    expectInlined(dedent`
      {
        id
      }
    `).to.equal(dedent`
      {
        id
      }
    `);
  });

[IvanGoncharov]

Use direct imports from appropriate files.

[IvanGoncharov]

Please use Object.create(null) here and use ObjMap for types.

[IvanGoncharov]

Remove empty line

[IvanGoncharov]

This is incorrect check, same fragment can be used in multiple places

{
  person: {
    ...ProfileInfo
    friend: {
      ...ProfileInfo
    }
  }
}

Also please cover in tests.

[acao]

covered in unit tests now :)

[IvanGoncharov]

I found a failing case that wasn't in the original unit tests:

I think I know the reason, I will fix this.

[langpavel]

Does this work well with aliases and experimental fragment variables?

[langpavel]

Why not Set?

[acao]

Set makes a lot more sense

[IvanGoncharov]

Map, Set should be used only for non-string keys, Object.create(null) is perfectly fine for working with string keys.

[langpavel]

@IvanGoncharov Set is much better primitive for checking presence/uniqueness than Object.create(null), both by math sense and by the fact — Maps and Sets are well tested and perfectly tuned for purpose. Misusing objects as hash maps, well, it's only relique. Polyfilling is not needed today.

[acao]

@langpavel good questions!

do you mean like this:

fragment UserFriends ($first: Int!, $after: Int) on User {
  name
  avatar
}

query {
   ...UserFriends(first: 100, after: null)
}

I don't see why it wouldn't, since this would still require visiting the same node types, but I can add some test cases to be sure

[IvanGoncharov]

Please review type definitions before you use them or convert them into each other.
Fragment definition has way more fields than inline fragments.

[acao]

for sure, adding more test cases, it can't be this simple

[IvanGoncharov]

You also removing duplicated fields, including fields with different aliases, args, etc.

[acao]

disabling until I've done more research

[benjie]

I have a more thorough implementation of this here: graphql/graphiql#1542

Let me know if you're interested into a PR to GraphQL.js

[IvanGoncharov]

@benjie I think it would make sense to test it in GraphiQL to refine API and implementation and latter move to graphql-js.
Also, the relay compiler supports this and several similar transformations so we can learn from them.