-
Notifications
You must be signed in to change notification settings - Fork 9
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
MWPW-152283: add acom wcs endpoint #12
Conversation
commerce/src/wcs.js
Outdated
const { env, wcsApiKey: apiKey } = settings; | ||
|
||
const { env, domainSwitch, wcsApiKey: apiKey } = settings; | ||
const baseUrl = domainSwitch ? WcsBaseUrl['STAGE_ACOM'] : WcsBaseUrl[env]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is problematic in case you are domainSwitch on and env production, no? i guess we won't have both normally, but still looks weird
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added acom prod too
commerce/src/settings.js
Outdated
@@ -217,6 +219,7 @@ function getSettings(config = {}) { | |||
entitlement, | |||
extraOptions: Defaults.extraOptions, | |||
modal, | |||
upgrade, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't mix things up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okie, will revert
commerce/src/settings.js
Outdated
@@ -180,6 +180,7 @@ function getSettings(config = {}) { | |||
Defaults.entitlement | |||
); | |||
const modal = toBoolean(getParameter('modal', commerce), Defaults.modal); | |||
const upgrade = toBoolean(getParameter('upgrade', commerce), Defaults.upgrade); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't mix things up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure domainSwitch
param was discussed before, but I think we should avoid introducing new params.
@@ -576,6 +576,7 @@ declare global { | |||
wcsBufferDelay: number; | |||
wcsBufferLimit: number; | |||
wcsEnv: Environment; | |||
domainSwitch: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we really need to introduce a new parameter with a confusing name:domainSwitch
?
I would like to avoid having to support temporarily introduced parameters for ever.
Can we just switch to this new domain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is just to verify options request will be gone.
I will remove it in the following PR when actually implementing the switch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yesil we need both domains to work so we can compare stuff from same domain
commerce/src/defaults.js
Outdated
@@ -22,6 +22,7 @@ export const Defaults = Object.freeze({ | |||
entitlement: false, | |||
extraOptions: {}, | |||
modal: false, | |||
upgrade: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please don't mix things up
@@ -576,6 +576,7 @@ declare global { | |||
wcsBufferDelay: number; | |||
wcsBufferLimit: number; | |||
wcsEnv: Environment; | |||
domainSwitch: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yesil we need both domains to work so we can compare stuff from same domain
Resolve https://jira.corp.adobe.com/browse/MWPW-152283
Oversimplified ability to test acom wcs domain performance (should take away OPTIONS request from stage.adobe.com)
To see this in action add
?commerce.env=STAGE&domain.switch=true
Test URLs: