-
Notifications
You must be signed in to change notification settings - Fork 381
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
feat: add OPF integration #17250
base: develop
Are you sure you want to change the base?
feat: add OPF integration #17250
Conversation
switchMap(([userId, cartId]: [string, string]) => { | ||
this.activeCartId = cartId; | ||
return this.cartAccessCodeService.getCartAccessCode(userId, cartId); | ||
}), | ||
filter((response) => Boolean(response?.accessCode)), | ||
map(({ accessCode: otpKey }) => | ||
this.setPaymentInitiationConfig(otpKey, paymentOptionId) | ||
), |
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.
[MEDIUM]
If possible, let's not mix static and reactive programming Here we're making a side-effect of setting a static property this.activeCartId
in the Observable flow, and later in this flow we read the value from the static property.
Mixing static and reactive programming is harder to reason about and also prone to issues in async edge cases.
-
I suggest to remove the property
protected activeCartId
, as not necessary. -
Then we can simply pass the
cartId
to the methodthis.setPaymentInitiationConfig()
, e.g. as a first argument:
switchMap(([userId, cartId]: [string, string]) => { | |
this.activeCartId = cartId; | |
return this.cartAccessCodeService.getCartAccessCode(userId, cartId); | |
}), | |
filter((response) => Boolean(response?.accessCode)), | |
map(({ accessCode: otpKey }) => | |
this.setPaymentInitiationConfig(otpKey, paymentOptionId) | |
), | |
switchMap(([userId, cartId]: [string, string]) => | |
this.cartAccessCodeService.getCartAccessCode(userId, cartId).pipe( | |
filter((response) => Boolean(response?.accessCode)), | |
map(({ accessCode: otpKey }) => | |
this.setPaymentInitiationConfig(cartId, otpKey, paymentOptionId) | |
) | |
) | |
), |
protected setPaymentInitiationConfig(
+ cartId: string,
otpKey: string,
paymentOptionId: number
) {
return {
otpKey,
config: {
configurationId: String(paymentOptionId),
- cartId: this.activeCartId,
+ cartId
selector: 'cx-opf-verify-payment', | ||
templateUrl: './opf-payment-verification.component.html', | ||
}) | ||
export class OpfPaymentVerificationComponent implements OnInit, OnDestroy { |
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.
[MAJOR]
JS of this component (and its indirect dependencies like OpfPaymentVerificationService
and OpfResourceLoaderService
) is now eagerly loaded in main.js on every page. It unnecessarily slows down the load of SEO-critical pages like homepage, PDP, PLP.
Note: it happens only when the OPF feature is installed by the customer.
Instead, I suggest to move out as much a spossible of the logic of this component to a lazy-loadable service/facade. Thanks to that, only the component's definition (and a tiny HTML template) will be loaded in main.js, but the actual logic of this component (and its indirect dependencies) will be lazy-loaded only when entering the payment-verification page.
constructor( | ||
protected launchDialogService: LaunchDialogService, | ||
protected el: ElementRef, | ||
protected cd: ChangeDetectorRef, | ||
protected opfErrorModalService: OpfErrorModalService | ||
) { |
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.
[MEDIUM]
According to our Coding Guidelines, in newly written source code, let's use inject()
function instead of constructors for Dependency Injection.
You might find this automated conversion tool useful: https://ngxtension.netlify.app/utilities/migrations/inject-migration/
No description provided.