Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Conversation

keharper
Copy link
Contributor

@keharper keharper commented May 2, 2019

Purpose of this pull request

This PR adds documentation for magento/graphql-ce#230 and internal ticket MC-15959

whatsnew
Added topics about GraphQL caching and identity classes.

@keharper keharper added Internal Dev Differentiates work between community and Magento staff New Topic A major update published as an entirely new document xx2.3.2 Magento 2.3.2 changes labels May 2, 2019
@keharper keharper self-assigned this May 2, 2019
@keharper keharper requested a review from cpartica May 2, 2019 16:41
@keharper
Copy link
Contributor Author

keharper commented May 2, 2019

This PR replaces #4260

@keharper keharper mentioned this pull request May 2, 2019
* `cart`
* `country`
* `countries`
* `currency`
Copy link
Member

Choose a reason for hiding this comment

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

Just out of curiosity, Why wouldn't it be valid to cache countries, country and currency?

Copy link

Choose a reason for hiding this comment

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

because it's not supported yet, the caching invalidation for system conf doesn't work with graphql

@osrecio osrecio self-assigned this May 3, 2019
@erikmarr erikmarr self-requested a review May 3, 2019 16:26
Copy link
Contributor

@erikmarr erikmarr left a comment

Choose a reason for hiding this comment

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

Just some minor suggestions/questions.

Co-Authored-By: keharper <keharper@users.noreply.github.com>
* `cart`
* `country`
* `countries`
* `currency`
Copy link

Choose a reason for hiding this comment

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

because it's not supported yet, the caching invalidation for system conf doesn't work with graphql

## Request headers

Magento accepts the following headers in a GraphQL request:

Header name | Value | Description
--- | --- | ---
`Authorization` | `Bearer <authorization_token>` | A customer token. [Get customer authorization token]({{ page.baseurl }}/graphql/get-customer-authorization-token.html) describes how to generate the token.
`Content-Currency` | A valid currency code, such as `USD` | This header is required only if the currency is not the store view's default currency.
Copy link

Choose a reason for hiding this comment

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

This information came in late but by doing Store & Content-Currency we're not repecting the caching RFC standard, which uses Vary instead as computed.
https://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html
https://tools.ietf.org/html/rfc7231#section-7.1.4

Content-Currency & Store is obviously not part of a standard,
and also we don't respond with a Vary. The frontend currently does, but it sets it in cookie not in headers
We don't respond with it at all, but we'll address this in a future story.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created internal ticket MC-16230 to track the doc work when the code changes.

@keharper keharper merged commit b4a42d3 into develop May 6, 2019
@ghost
Copy link

ghost commented May 6, 2019

Hi @keharper, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@keharper keharper deleted the kh_hb-caching branch May 6, 2019 21:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Internal Dev Differentiates work between community and Magento staff New Topic A major update published as an entirely new document xx2.3.2 Magento 2.3.2 changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants