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

acao · 2019-06-04T05:25:22Z

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 · 2019-06-04T06:41:00Z

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 · 2019-06-04T07:05:08Z

src/utilities/mergeAST.js

+ // Collect the existing FragmentDefinition nodes.
+ fragmentDefinitions[node.name.value] = node;
+ },
+ });


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

IvanGoncharov · 2019-06-04T07:11:46Z

src/utilities/__tests__/mergeAST-test.js

+ it(fixture.desc, () => {
+ const result = print(mergeAST(parse(fixture.query))).replace(/\s/g, '');
+ expect(result).to.equal(fixture.mergedQuery.replace(/\s/g, ''));
+ });


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

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 · 2019-06-04T07:12:29Z

src/utilities/__tests__/mergeAST-test.js

+import { expect } from 'chai';
+import { describe, it } from 'mocha';
+
+import { parse, print } from '../../index';


Use direct imports from appropriate files.

IvanGoncharov · 2019-06-04T07:15:05Z

src/utilities/mergeAST.js

+
+export function mergeAST(ast: DocumentNode): DocumentNode {
+ const fragmentDefinitions: { [name: string]: FragmentDefinitionNode } = {};
+ const inlinedFragments: { [name: string]: FragmentSpreadNode } = {};


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

IvanGoncharov · 2019-06-04T07:16:26Z

src/utilities/mergeAST.js

+ * Given a document AST, merge all fragment definitions into the
+ * provided operations using inline fragments.
+ */
+


Remove empty line

IvanGoncharov · 2019-06-04T07:24:34Z

src/utilities/mergeAST.js

+ return visit(ast, {
+ FragmentSpread(node) {
+ // No repeats!
+ if (inlinedFragments[node.name.value]) {


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

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

Also please cover in tests.

covered in unit tests now :)

IvanGoncharov · 2019-06-04T07:30:40Z

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 · 2019-06-04T12:24:04Z

src/jsutils/uniqueBy.js

+ array: $ReadOnlyArray<any>,
+ iteratee: (item: any) => any,
+) {
+ const FilteredMap = new Map();


Why not Set?

Set makes a lot more sense

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

@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 · 2019-06-04T14:04:32Z

@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 · 2019-06-04T15:23:31Z

src/utilities/inlineNamedFragments.js

+ return {
+ ...fragmentDefinitions[node.name.value],
+ kind: 'InlineFragment',
+ };


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

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

IvanGoncharov · 2019-06-04T15:24:57Z

src/utilities/inlineNamedFragments.js

+ ...node,
+ selections: uniqueBy(
+ node.selections,
+ selection => selection.name.value,


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

acao · 2019-06-04T16:10:03Z

disabling until I've done more research

benjie · 2020-05-21T14:33:53Z

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 · 2020-05-21T18:04:29Z

@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.

acao force-pushed the add-refactor-mergeAST branch from 023aa09 to f78ec10 Compare June 4, 2019 05:27

acao mentioned this pull request Jun 4, 2019

@types/graphql - provide type definitions for mergeAST utility DefinitelyTyped/DefinitelyTyped#35942

Closed

acao force-pushed the add-refactor-mergeAST branch from f78ec10 to 19a8337 Compare June 4, 2019 05:45

IvanGoncharov reviewed Jun 4, 2019

View reviewed changes

acao force-pushed the add-refactor-mergeAST branch from 19a8337 to 5d3b34a Compare June 4, 2019 08:24

IvanGoncharov added the PR: feature 🚀 requires increase of "minor" version number label Jun 4, 2019

acao force-pushed the add-refactor-mergeAST branch 5 times, most recently from 8199f23 to 8d1d14c Compare June 4, 2019 10:15

add mergeAST from GraphiQL, refactor using visit()

997ead9

acao force-pushed the add-refactor-mergeAST branch from 8d1d14c to 997ead9 Compare June 4, 2019 10:18

langpavel suggested changes Jun 4, 2019

View reviewed changes

IvanGoncharov reviewed Jun 4, 2019

View reviewed changes

acao changed the title ~~add mergeAST from GraphiQL, refactor using visit()~~ add mergeAST from GraphiQL, refactor using visit() [WIP] Jun 4, 2019

acao closed this Jun 4, 2019

snyk-bot mentioned this pull request Dec 7, 2021

[Snyk] Upgrade cspell from 5.12.3 to 5.12.6 turkdevops/graphql-js#382

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

acao commented Jun 4, 2019 •

edited

Loading

acao commented Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

IvanGoncharov Jun 4, 2019

acao Jun 4, 2019

IvanGoncharov commented Jun 4, 2019

langpavel left a comment

langpavel Jun 4, 2019

acao Jun 4, 2019

IvanGoncharov Jun 4, 2019

langpavel Jun 4, 2019

acao commented Jun 4, 2019 •

edited

Loading

IvanGoncharov Jun 4, 2019

acao Jun 4, 2019

IvanGoncharov Jun 4, 2019

acao commented Jun 4, 2019

benjie commented May 21, 2020

IvanGoncharov commented May 21, 2020

Labels

4 participants

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

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

Uh oh!

Conversation

acao commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

acao commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanGoncharov commented Jun 4, 2019

langpavel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Jun 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acao commented Jun 4, 2019

benjie commented May 21, 2020

IvanGoncharov commented May 21, 2020

Labels

4 participants

acao commented Jun 4, 2019 •

edited

Loading

acao commented Jun 4, 2019 •

edited

Loading