Skip to content

Commit

Permalink
Don't import locally defined fragment types
Browse files Browse the repository at this point in the history
Reviewed By: kassens

Differential Revision: D6849018

fbshipit-source-id: 84636771b23c9285f49a13527162613d8d7edd17
  • Loading branch information
devknoll authored and facebook-github-bot committed Jan 30, 2018
1 parent 970c254 commit 7985449
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 6 deletions.
17 changes: 11 additions & 6 deletions packages/relay-compiler/core/RelayFlowGenerator.js
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ type Options = {|

export type State = {|
...Options,
+generatedFragments: Set<string>,
+usedEnums: {[name: string]: GraphQLEnumType},
+usedFragments: Set<string>,
|};
Expand Down Expand Up @@ -228,6 +229,7 @@ function createVisitor(options: Options) {
customScalars: options.customScalars,
enumsHasteModule: options.enumsHasteModule,
existingFragmentNames: options.existingFragmentNames,
generatedFragments: new Set(),
inputFieldWhiteList: options.inputFieldWhiteList,
relayRuntimeModule: options.relayRuntimeModule,
usedEnums: {},
Expand Down Expand Up @@ -270,6 +272,7 @@ function createVisitor(options: Options) {
}
return [selection];
});
state.generatedFragments.add(node.name);
const refTypeName = getRefTypeName(node.name);
const refType = t.expressionStatement(
t.identifier(
Expand Down Expand Up @@ -406,12 +409,14 @@ function getFragmentImports(state: State) {
const usedFragments = Array.from(state.usedFragments).sort();
for (const usedFragment of usedFragments) {
const refTypeName = getRefTypeName(usedFragment);
if (state.useHaste && state.existingFragmentNames.has(usedFragment)) {
// TODO(T22653277) support non-haste environments when importing
// fragments
imports.push(importTypes([refTypeName], usedFragment + '.graphql'));
} else {
imports.push(anyTypeAlias(refTypeName));
if (!state.generatedFragments.has(usedFragment)) {
if (state.useHaste && state.existingFragmentNames.has(usedFragment)) {
// TODO(T22653277) support non-haste environments when importing
// fragments
imports.push(importTypes([refTypeName], usedFragment + '.graphql'));
} else {
imports.push(anyTypeAlias(refTypeName));
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -359,6 +359,25 @@ export type PluralFragment = $ReadOnlyArray<{|
|}>;
`;
exports[`RelayFlowGenerator matches expected output: recursive-fragments.graphql 1`] = `
~~~~~~~~~~ INPUT ~~~~~~~~~~
fragment FragmentSpread on Node {
id
... @include(if: $condition) {
...FragmentSpread
}
}
~~~~~~~~~~ OUTPUT ~~~~~~~~~~
import type { FragmentReference } from 'relay-runtime';
declare export opaque type FragmentSpread$ref: FragmentReference;
export type FragmentSpread = {|
+id: string,
+$fragmentRefs: FragmentSpread$ref,
+$refType: FragmentSpread$ref,
|};
`;
exports[`RelayFlowGenerator matches expected output: roots.graphql 1`] = `
~~~~~~~~~~ INPUT ~~~~~~~~~~
query ExampleQuery($id: ID!) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
fragment FragmentSpread on Node {
id
... @include(if: $condition) {
...FragmentSpread
}
}

3 comments on commit 7985449

@kastermester
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the motivation behind this commit?

Is it somehow possible to make circular references between fragments now? I have looked at the graphql-js repo and here for anything indicating this behavior and couldn't find anything.

@jstejada
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @devknoll, I believe it is possible to recursively reference fragments now

@devknoll
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With fragment arguments (and constant folding), it's possible that queries that are self-referential will be compiled down to valid queries. For that reason, we loosened the restriction a bit.

Please sign in to comment.