-
Notifications
You must be signed in to change notification settings - Fork 219
feat(payment): PAYPAL-4935 added CartActionCreator for handling/storing cart information #2802
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
base: master
Are you sure you want to change the base?
Conversation
…ng cart information
constructor(private _cartRequestSender: CartRequestSender) {} | ||
|
||
@cachableAction | ||
loadCard( |
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.
loadCard( | |
loadCart( |
options?: RequestOptions & ActionOptions, | ||
): ThunkAction<LoadCartAction, InternalCheckoutSelectors> { | ||
return (store) => { | ||
return Observable.create((observer: Observer<LoadCartAction>) => { |
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 think Observable.create
is deprecated. You might need to use new Observable
here?
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.
Make sense, thanks
observer.next(createAction(CartActionType.LoadCartRequested, undefined)); | ||
|
||
this._cartRequestSender | ||
.loadCard(cartId, host, options) |
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.
.loadCard(cartId, host, options) | |
.loadCart(cartId, host, options) |
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 have a general question around is it needed to add methods to loadCart
, since we already gave loadCheckout
. Cart is a sub entity of checkout so if we fetch checkout we can extract the cart object from there.
@animesh1987 yeap, you are right. We need to be able to get some information from |
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.
To make things future-proof, we should consider doing loadCheckout instead of loadCart. This change could be incorporated into this PR.
@@ -75,4 +78,35 @@ describe('CartRequestSender', () => { | |||
}); | |||
}); | |||
}); | |||
|
|||
describe('#loadCard', () => { |
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.
describe('#loadCard', () => { | |
describe('#loadCart', () => { |
@bc-peng thanks for your comment. I want to separate this functionality to another PR because of huge changes/updates, so this part of the work needs to be separated. But this PR would be better to leave as it is. cc @animesh1987 |
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.
Overall looks good to me 👍
packages/core/src/cart/headless-cart/mocks/headless-cart.mock.ts
Outdated
Show resolved
Hide resolved
options?: RequestOptions, | ||
): Promise<Response<Cart | undefined>> { | ||
const path = 'cart-information'; | ||
const url = host ? `${host}/${path}` : `/${path}`; |
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.
If there is no host, then are we calling https://<some-domain>/cart-information
? Where is this endpoint implemented?
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.
For default host will be taken as window.location.origin when we are making an request. This endpoint is implemented in the Catalyst project using the nextjs route function
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.
Out of curiosity why are we not calling cart GQL get directly?
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.
Because we have a problem with third party cookies if we try to make request directly. To avoid this problem, we should make the request as server to server
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 know who owns this endpoint and how it works?
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.
@davidchin here is the line
As far as I understood we have to provide ability to change host by client but default path will be the same. It means client is not be able to change api/wallet-buttons/cart-information
but only host
. We have other endpoints that we will work with in a similar fashion such as api/wallet-buttons/get-initialization-data
, api/wallet-buttons/get-customer
, api/wallet-buttons/load-checkout
and so on. Do I understand you correctly?
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 have to provide a default value for each of the endpoints, and it doesn't matter if the host is set or not. But based on your suggestion we have to create config for each endpoint to handle it properly.
For example create a config as following:
const graphQLRequestConfig = {
GET_CHECKOUT: 'get-checkout-path' ,
GET_CART_INFORMATION: 'get-cart-path',
...
}
We should save this config to the store during wallet button initialization, saving it to configMetaState as an example.
If we do not have graphQLRequestConfig.GET_CART_INFORMATION
, for example, we will set up default url. @davidchin what do you think?
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.
As far as I understood we have to provide ability to change host by client but default path will be the same.
I was referring to the path rather than the host, as requests are expected to be sent to the same host (e.g.: Catalyst storefront) rather than a different one. Depending on the headless implementation, the path may differ from the default path.
We have to provide a default value for each of the endpoints
I wasn't aware that we needed to expose so many REST endpoints from Catalyst. Taking a step back, I believe we should aim to keep the Catalyst API layer as thin as possible. The reasoning is similar to the above: Catalyst serves as a reference implementation for headless storefronts. Once it is forked, BC has no control over it, meaning we can't update the APIs. Imagine if a new payment wallet requires an additional endpoint or payload, we wouldn't be able to retroactively add it to existing headless Catalyst storefronts.
Instead, could we create a single endpoint that proxies GraphQL requests to the our Storefront GraphQL API? The proxy endpoint would only need to perform a session check to ensure the caller has permission to access the requested data. This approach would be more scalable IMO, as Catalyst would be responsible only for session validation, while BC would handle the actual requests.
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 see what you mean. I am not sure that we can make it using one enpoint unfortunately. We have a list of endpoints and we have to define each of them separately. Perhaps one possible way to solve this problem is to create a config that will contain a list of endpoints as i referred here. @davidchin what do you think?
UPD: And based on what you shared with me we have to pass not only request payload but also full graphql query
from checkout-sdk
to Catalyst proxy server
because this data can be updated at any time for the purposes we need from checkout-sdk
not from Catalyst proxy server
otherwise these changes will not be available to Catalyst storefronts that were forked before. Am I right? If I am, it will not be a big problem to move graphql queries
to checkout-sdk-js
.
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.
@davidchin @animesh1987 I've updated the PR with changes related to our last conversation about specifying a URL when initializing the wallet buttons and setting a default value if it wasn't specified. Take a look please 🙏
60386a5
to
9ddbcdc
Compare
1d7a044
to
90a4858
Compare
What?
Added
CartActionCreator
for handling/storing cart informationBased on POC
Why?
In scope of Headless Wallet Buttons project
Testing / Proof
Manual testing
@bigcommerce/team-checkout @bigcommerce/team-payments