Skip to content
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

__resolveReference follow-ups #1757

Merged
merged 4 commits into from
Apr 22, 2022
Merged

__resolveReference follow-ups #1757

merged 4 commits into from
Apr 22, 2022

Conversation

trevor-scheer
Copy link
Member

Addressing feedback from #1746

The vague "__" is unhelpful here. We're not expecting a bunch of things to be prefixed with __ (it violates the spec to do so, albeit a useful hack in the past https://spec.graphql.org/October2021/#sec-Objects.Type-Validation).

  • Adjust the string to look for specifically the things we expect (__resolveReference and __resolveType)
  • Cleanup the duplication of schema-helper (which is only needed for gateway's test code, not runtime)

@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for apollo-federation-docs canceled.

Name Link
🔨 Latest commit 29ae85c
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/6261ea742459280008197be3

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 21, 2022

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

},
};
} else if (fieldName.startsWith("__")) {
(type as any)[fieldName.substring(2)] = fieldConfig;
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

Note to reviewer: I restored this continue in the resolveReference case, which previously existed here: https://github.com/apollographql/federation/pull/1746/files#diff-89b6c52a33464884610c771bf9c04484fc8acf56fb57fd9a31b168dd55595530R181

Bit tough to see with the diff. No effect on tests with or without.

Copy link
Member

Choose a reason for hiding this comment

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

Good catch — sorry for missing that in the last review. I guess technically a no-op since fieldMap['__resolveReference'] shouldn't be truthy.

gateway-js/src/__tests__/executeQueryPlan.test.ts Outdated Show resolved Hide resolved
gateway-js/tsconfig.test.json Outdated Show resolved Hide resolved
},
};
} else if (fieldName.startsWith("__")) {
(type as any)[fieldName.substring(2)] = fieldConfig;
continue;
Copy link
Member

Choose a reason for hiding this comment

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

Good catch — sorry for missing that in the last review. I guess technically a no-op since fieldMap['__resolveReference'] shouldn't be truthy.

* Support __isTypeOf
* Reference src instead of dist aross packages
@trevor-scheer trevor-scheer requested a review from glasser April 21, 2022 23:38
@trevor-scheer trevor-scheer merged commit 8a5338c into main Apr 22, 2022
@trevor-scheer trevor-scheer deleted the trevor/cleanup-1746 branch April 22, 2022 00:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants