-
Notifications
You must be signed in to change notification settings - Fork 215
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
chore(common): CHECKOUT-6855 Load payment strategies from V2 as priority #1529
Conversation
@@ -19,6 +21,7 @@ import { PaymentStrategy } from './strategies'; | |||
export default class PaymentStrategyActionCreator { | |||
constructor( | |||
private _strategyRegistry: PaymentStrategyRegistry, | |||
private _strategyRegistryV2: ResolveIdRegistry<PaymentStrategyV2, PaymentStrategyResolveId>, |
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.
You could import PaymentStrategyRegistry
from payment-strategy-registry-v2
, which is essentially an alias for ResolveIdRegistry<PaymentStrategyV2, PaymentStrategyResolveId>
.
return strategy | ||
.execute(payload, { ...options, methodId: payment.methodId, gatewayId: payment.gatewayId }) | ||
.then(() => createAction(PaymentStrategyActionType.ExecuteSucceeded, undefined, meta)); | ||
const promise: Promise<InternalCheckoutSelectors | void> = strategy.execute(payload, { ...options, methodId: payment.methodId, gatewayId: payment.gatewayId }); |
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.
You probably can keep what's there before instead of assigning to a temp variable.
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 had to do this since typescript was confused about mixed return types since either of the strategy methods had their return types not compatible, so it's essentially setting a type.
strategy = this._strategyRegistry.getByMethod(method) | ||
} | ||
|
||
const promise: Promise<InternalCheckoutSelectors | void> = strategy.initialize({ ...options, methodId, gatewayId }); |
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.
Likewise here.
@@ -44,7 +47,17 @@ async function getBaseConfig() { | |||
}, | |||
{ | |||
test: /\.[tj]s$/, | |||
include: srcPath, | |||
include: coreSrcPath, |
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.
Is there a way we can specify the ts-loader
without having to do it individually for each package?
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 linked the ts-loader issue in the PR, we can try use tsconfig includes going forward but was not sure if we need to upgrade ts-loader for that.
I am going to move this config to a new file so we can have all the config object in one place.
Another way is what we discussed about moving builds to each package, but that may require thinking around how to export types.
51f8db4
to
d8c723b
Compare
What?
Why?
As we move towards payment strategy packages, we want to register strategies as part of build process.
Also process packages with ts-loader, we need to have a rule per package for now issue.
Testing / Proof
@bigcommerce/checkout