-
Notifications
You must be signed in to change notification settings - Fork 174
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-156672: refactor commerce & optimize pandora dependencies #2723
Conversation
Hello, I'm the AEM Code Sync Bot and I will run some actions to deploy your branch and validate page speed.
|
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## stage #2723 +/- ##
==========================================
- Coverage 95.90% 95.89% -0.02%
==========================================
Files 173 173
Lines 45842 45842
==========================================
- Hits 43967 43958 -9
- Misses 1875 1884 +9 ☔ View full report in Codecov by Sentry. |
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.
fancy, is it also planned to support subdomains? E.g. same domain requests from business.adobe.com
todo: test on adobe-students |
from business.adobe.com most likely browser will initiate a preflight, since its a different domain, but without testing I can't say for sure. |
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.
these are good changes that are needed, but I find it a bit risky to combine it with CDN change.
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.
No local issues sounds nice :) I'll let the MAS people approve this, as I don't have sufficient context.
let wcsURL = 'https://www.adobe.com/web_commerce_artifact'; | ||
if (lowHostEnv) { | ||
wcsURL = 'https://www.stage.adobe.com/web_commerce_artifact'; |
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.
Nit: maybe string repetition could be avoided by determining the environment and then using a template literal to form the URL? Or maybe even a simple replace
?
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.
refactored a bit
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.
smaller PR: #2725 |
Reminder to set the |
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
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.
File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
added some changes on this PR so will need a re-review:
|
"@pandora/react-auth-provider": "file:./internal/react-auth-provider-1.2.1.tgz", | ||
"@pandora/react-env-provider": "file:./internal/react-env-provider-1.2.2.tgz", | ||
"@pandora/react-fetch-provider": "file:./internal/react-fetch-provider-1.2.2.tgz" | ||
"@pandora/logger": "file:./internal/logger-1.3.0.tgz" |
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.
we should not depend on this anymore.
Did you try removing package-lock.json
and npm install
againg?
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.
yes, the checkout builder is pulling the data source package, which pulls logger etc
@@ -5,12 +5,6 @@ import { | |||
buildCheckoutUrl, | |||
} from '@pandora/commerce-checkout-url-builder'; | |||
import { Term, Commitment } from '@pandora/data-models-odm'; |
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.
we don't need these imports/we can replace them, I propose to remove them as well.
/** | ||
* Cache of promises resolving to arrays of Wcs offers grouped by osi-based keys. | ||
* @type {Map<string, Promise<Commerce.Wcs.Offer[]>>} | ||
*/ | ||
const cache = new Map(); | ||
/** | ||
* Queue of pending requests to Wcs grouped by locale and promo. | ||
* @type {Map<string, { options: WcsOptions, promises: WcsPromises }>} |
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.
We could define WcsOptions
here in this file as well.
This PR is a follow up of #2725 and:
Resolves: https://jira.corp.adobe.com/browse/MWPW-156672
Original discussion: https://git.corp.adobe.com/orgs/wcms/discussions/38
Test URLs: