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

Add specific language to context docs that reference resolvers are necessary for contextual fields #3102

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

clenfest
Copy link
Contributor

No description provided.

@clenfest clenfest requested a review from a team as a code owner July 31, 2024 16:58
Copy link

changeset-bot bot commented Jul 31, 2024

⚠️ No Changeset found

Latest commit: e6087ba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Jul 31, 2024

Deploy Preview for apollo-federation-docs failed.

Name Link
🔨 Latest commit e6087ba
🔍 Latest deploy log https://app.netlify.com/sites/apollo-federation-docs/deploys/66bbb7d99837ad00082fbafe

Copy link

codesandbox-ci bot commented Jul 31, 2024

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.

Copy link
Contributor

@Meschreiber Meschreiber left a comment

Choose a reason for hiding this comment

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

Thanks for the addition @clenfest . I pushed some copyedits and have some inline clarifying questions.

Finally, it seems like this is a prerequisite and should be mentioned earlier, for example in a Note at the top of the relevant section(s). (E.g.:

Note: Fields that are used with the context directives must be resolvable via a reference resolver. [Learn more](#link-to-section).

Does this apply to just Referencing fields across subgraphs or to all sections on the page?


Fields that are used with the context directives must be resolvable via a [reference resolver](/federation/entities/#2-define-a-reference-resolver). Query plans that fetch contextual fields are based on the entity where the `@context` directive is set. Therefore, all subgraphs that resolve that entity must support looking up their fields based on the entity's `@keys` fields.

Take the [previous example](#referencing-fields-across-subgraphs). Suppose the first subgraph has a single query to retrieve the logged in user:
Copy link
Contributor

Choose a reason for hiding this comment

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

Added this anchor, as I assumed that's the example we're talking about. Could you please confirm @clenfest ?

```typescript title="❌ Incorrect example" disableCopy showLineNumbers=false
Query: {
me: () => ({
id: 1, userCurrency: { id: 431, currencyCode: 'USD'}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be isoCode?

Suggested change
id: 1, userCurrency: { id: 431, currencyCode: 'USD'}
id: 1, userCurrency: { id: 431, isoCode: 'USD'}

}
```

This is because the fetch set to subgraph 1 to retrieve the `isoCode` would be done on the entity. Therefore a section to resolve a User by keys would need to be added:
Copy link
Contributor

@Meschreiber Meschreiber Aug 13, 2024

Choose a reason for hiding this comment

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

This sentence confuses me. Does it mean something like this:

Suggested change
This is because the fetch set to subgraph 1 to retrieve the `isoCode` would be done on the entity. Therefore a section to resolve a User by keys would need to be added:
It's an inadequate resolver because retrieving the `isoCode` relies on the retrieving the `User` entity itself. Therefore, your resolvers file must include a reference resolver for the `User` entity:

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm also a bit confused by the examples--the incorrect example shows just a resolver for the me query, while the addition below is a reference resolver for User. Would that me query resolver work as written with just the addition? Or does it have to be updated as well?

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