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

USF-1877: Headers from config #267

Merged
merged 16 commits into from
Jan 15, 2025
Merged

USF-1877: Headers from config #267

merged 16 commits into from
Jan 15, 2025

Conversation

sirugh
Copy link
Collaborator

@sirugh sirugh commented Dec 20, 2024

Note!

The Ask

The headers and query parameters used by the PDP dropin are stored in the content space in config files. These values are read per key:

setFetchGraphqlHeaders({
  'Content-Type': 'application/json',
  'x-api-key': await getConfigValue('commerce-x-api-key')
});

Therefore, new query params or headers must be added to the config AND to the codebase.

setFetchGraphqlHeaders({
  'Content-Type': 'application/json',
  'x-api-key': await getConfigValue('commerce-x-api-key'),
  'some-new-header': await getConfigValue('some-new-header-key') // <-- This isn't great!
});

We want these headers to be managed entirely in config rather than requiring developers to make code changes.

The Solution

We can add config values with some consistent path, ie commerce.headers.[scope].[header] where scope is like pdp and header is the string header key. The corresponding value will be injected as header or query param, programatically.

// if config has "commerce.headers.pdp.foo": "bar"
// then the following should assign { "foo": "bar" }
setFetchGraphqlHeaders(await getHeadersFor('pdp'));

image

The Downside

Assuming headers will be defined in this way, then there is potential for duplication in the config. For example if two dropins require the same value, there will be duplicate values, for example:

// both have same value in config
getConfigValue("commerce.headers.pdp.Magento-Environment-Id") 
getConfigValue("commerce.headers.checkout.Magento-Environment-Id")

Additionally, there are places where these values are used but not as headers. For example, here used to be getConfigValue('commerce-customer-group) but we removed that entry to reduce duplication in config. However this results in coupling of that line to the cs.Magento-Customer-Group header in config.

Test URLs:

Validate this on any of the following:

Copy link

aem-code-sync bot commented Dec 20, 2024

Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
In case there are problems, just click a checkbox below to rerun the respective action.

  • Re-run PSI checks
  • Re-sync branch
Commits

Copy link

aem-code-sync bot commented Dec 20, 2024

sirugh added 2 commits January 8, 2025 10:28
Signed-off-by: Stephen Rugh <[email protected]>
@@ -42,7 +42,7 @@ export default async function decorate(block) {
},
},
context: {
customerGroup: await getConfigValue('commerce-customer-group'),
customerGroup: await getConfigValue('commerce.queryparam.cs.Magento-Customer-Group'),
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we dedupe the config, so that we only have values in one place when possible, then we will end up with quite a lot of these sort of calls, where the key doesn't really make sense in the context of what we're passing the value to.

So it's a choice, I guess. Either we keep multiple duplicated values within the config, so the keys are more explicit, or we remove extra values and rely on just a single config value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally the PLP and LS widget are updated to allow for passing of headers in a single headers object rather than as top-level props on this "config" object. Then we could use the same iteration logic as the PDP dropin in this PR.

@sirugh sirugh changed the title poc: headers from config Headers from config Jan 8, 2025
@sirugh sirugh marked this pull request as ready for review January 8, 2025 19:19
@sirugh sirugh changed the title Headers from config USF-1877: Headers from config Jan 13, 2025
@sirugh sirugh changed the title USF-1877: Headers from config USF-1877: Headers/Quer Parameters from config Jan 14, 2025
@sirugh sirugh changed the title USF-1877: Headers/Quer Parameters from config USF-1877: Headers/Query Parameters from config Jan 14, 2025
Signed-off-by: Stephen Rugh <[email protected]>
Signed-off-by: Stephen Rugh <[email protected]>
@sirugh sirugh changed the title USF-1877: Headers/Query Parameters from config USF-1877: Headers from config Jan 15, 2025
urlWithQueryParams.searchParams.append('Magento-Store-Code', await getConfigValue('commerce-store-code'));
urlWithQueryParams.searchParams.append('Magento-Customer-Group', await getConfigValue('commerce-customer-group'));
// Set some query parameters for use as a cache-buster. No other purpose.
urlWithQueryParams.searchParams.append('ac-storecode', await getConfigValue('commerce.headers.cs.Magento-Store-Code'));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

major change - we are reverting back to rely on headers for request data differentation. the only query param we add is now done for cache busting purposes.

tools/picker/src/index.js Outdated Show resolved Hide resolved
@sirugh sirugh merged commit 4d736e8 into main Jan 15, 2025
3 checks passed
@sirugh sirugh deleted the USF-1877 branch January 15, 2025 18:03
@herzog31 herzog31 added the changelog Important when updating projects label Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog Important when updating projects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants