-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add mergeAST from GraphiQL, refactor using visit() [WIP] #1948
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow strict | ||
*/ | ||
|
||
/** | ||
* Returns an array of unique values based on iteratee | ||
* which is invoked for each element in array to generate | ||
* the criterion by which uniqueness is computed. | ||
* | ||
* Simiar to _.uniqBy from lodash. | ||
*/ | ||
export function uniqueBy( | ||
array: $ReadOnlyArray<any>, | ||
iteratee: (item: any) => any, | ||
) { | ||
const FilteredMap = new Map(); | ||
const result = []; | ||
for (const item of array) { | ||
const uniqeValue = iteratee(item); | ||
if (!FilteredMap.has(uniqeValue)) { | ||
FilteredMap.set(uniqeValue, true); | ||
result.push(item); | ||
} | ||
} | ||
return result; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,189 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow strict | ||
* | ||
*/ | ||
|
||
export const fixtures = [ | ||
{ | ||
desc: 'does not modify query with no fragments', | ||
query: ` | ||
{ | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
id | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'inlines simple nested fragment', | ||
query: ` | ||
{ | ||
...Fragment1 | ||
} | ||
|
||
fragment Fragment1 on Test { | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
... on Test { | ||
id | ||
} | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'inlines triple nested fragment', | ||
query: ` | ||
{ | ||
...Fragment1 | ||
} | ||
|
||
fragment Fragment1 on Test { | ||
...Fragment2 | ||
} | ||
|
||
fragment Fragment2 on Test { | ||
...Fragment3 | ||
} | ||
|
||
fragment Fragment3 on Test { | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
... on Test { | ||
... on Test { | ||
... on Test { | ||
id | ||
} | ||
} | ||
} | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'inlines multiple fragments', | ||
query: ` | ||
{ | ||
...Fragment1 | ||
...Fragment2 | ||
...Fragment3 | ||
} | ||
|
||
fragment Fragment1 on Test { | ||
id | ||
} | ||
|
||
fragment Fragment2 on Test { | ||
id | ||
} | ||
|
||
fragment Fragment3 on Test { | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
... on Test { | ||
id | ||
} | ||
... on Test { | ||
id | ||
} | ||
... on Test { | ||
id | ||
} | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'inlines multiple fragments on multiple queries', | ||
query: ` | ||
{ | ||
...Fragment4 | ||
...Fragment5 | ||
} | ||
|
||
fragment Fragment5 on Test1 { | ||
...Fragment4 | ||
} | ||
|
||
fragment Fragment4 on Test1 { | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
... on Test1 { | ||
id | ||
} | ||
... on Test1 { | ||
... on Test1 { | ||
id | ||
} | ||
} | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'reuses the same fragment', | ||
query: ` | ||
fragment ProfileInfo on Person { | ||
name | ||
title | ||
phone | ||
} | ||
|
||
{ | ||
person { | ||
...ProfileInfo | ||
friend { | ||
...ProfileInfo | ||
} | ||
} | ||
}`, | ||
resultQuery: ` | ||
{ | ||
person { | ||
... on Person { | ||
name | ||
title | ||
phone | ||
} | ||
friend { | ||
... on Person { | ||
name | ||
title | ||
phone | ||
} | ||
} | ||
} | ||
} | ||
`, | ||
}, | ||
{ | ||
desc: 'removes duplicate fragment spreads', | ||
query: ` | ||
{ | ||
...Fragment1 | ||
...Fragment1 | ||
} | ||
|
||
fragment Fragment1 on Test { | ||
id | ||
}`, | ||
resultQuery: ` | ||
{ | ||
... on Test { | ||
id | ||
} | ||
} | ||
`, | ||
}, | ||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow strict | ||
*/ | ||
|
||
import { expect } from 'chai'; | ||
import { describe, it } from 'mocha'; | ||
import dedent from '../../jsutils/dedent'; | ||
import { parse } from '../../language/parser'; | ||
import { print } from '../../language/printer'; | ||
import { inlineNamedFragments } from '../inlineNamedFragments'; | ||
import { fixtures } from './inlineNamedFragments-fixture'; | ||
|
||
describe('inlineNamedFragments', () => { | ||
fixtures.forEach(fixture => { | ||
it(fixture.desc, () => { | ||
const result = print(inlineNamedFragments(parse(fixture.query))); | ||
expect(result).to.equal(dedent(fixture.resultQuery)); | ||
}); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,52 @@ | ||
/** | ||
* Copyright (c) Facebook, Inc. and its affiliates. | ||
* | ||
* This source code is licensed under the MIT license found in the | ||
* LICENSE file in the root directory of this source tree. | ||
* | ||
* @flow strict | ||
*/ | ||
|
||
import { type ObjMap } from '../jsutils/ObjMap'; | ||
import { uniqueBy } from '../jsutils/uniqueBy'; | ||
import { visit } from '../language/visitor'; | ||
import { | ||
type DocumentNode, | ||
type FragmentDefinitionNode, | ||
} from '../language/ast'; | ||
|
||
/** | ||
* Given a document AST, inline all named fragment definitions | ||
*/ | ||
export function inlineNamedFragments(documentAST: DocumentNode): DocumentNode { | ||
const fragmentDefinitions: ObjMap<FragmentDefinitionNode> = Object.create( | ||
null, | ||
); | ||
|
||
for (const definition of documentAST.definitions) { | ||
if (definition.kind === 'FragmentDefinition') { | ||
fragmentDefinitions[definition.name.value] = definition; | ||
} | ||
} | ||
|
||
return visit(documentAST, { | ||
FragmentSpread(node) { | ||
return { | ||
...fragmentDefinitions[node.name.value], | ||
kind: 'InlineFragment', | ||
}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please review type definitions before you use them or convert them into each other. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for sure, adding more test cases, it can't be this simple |
||
}, | ||
SelectionSet(node) { | ||
return { | ||
...node, | ||
selections: uniqueBy( | ||
node.selections, | ||
selection => selection.name.value, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You also removing duplicated fields, including fields with different aliases, args, etc. |
||
), | ||
}; | ||
}, | ||
FragmentDefinition() { | ||
return null; | ||
}, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not
Set
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set
makes a lot more senseThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Map
,Set
should be used only for non-string keys,Object.create(null)
is perfectly fine for working with string keys.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanGoncharov
Set
is much better primitive for checking presence/uniqueness thanObject.create(null)
, both by math sense and by the fact —Map
s andSet
s are well tested and perfectly tuned for purpose. Misusing objects as hash maps, well, it's only relique. Polyfilling is not needed today.