[commerce-sdk-react] Consume parameter keys (@W-15676839@) #1823
Conversation
|
|
||
| export default { | ||
| getBasket, | ||
| getPaymentMethodsForBasket, |
There was a problem hiding this comment.
@kevinxh The ticket asks us to consider whether the change will be a breaking one or not? What specifically made us to think that?
So far I have not seen anything concerning. For example, this file deletion should be ok, right? These param keys were not publicly exported.
There was a problem hiding this comment.
I think it's okay to remove this file.
We specifically exports the public interface from the index file. Anything not exported from that file is considered private.
|
|
||
| // Parameters can be set in `apiOptions` or `client.clientConfig`; | ||
| // we must merge them in order to generate the correct query key. | ||
| const netOptions = omitNullableParameters(mergeOptions(client, apiOptions)) | ||
| // get param keys for the api from netOptions | ||
| const paramKeys = [...paramKeysMap[methodName], ...getCustomKeys(netOptions.parameters)] | ||
| const paramKeys = [ |
There was a problem hiding this comment.
I am wondering if we can create a util that returns the keys including custom ones. 🤔 . With this we can optimise a few places (here and queryKeyHelper.queryKeys)
const paramKeys = getParamKeys(medthodName, params)
// util.js
const getParamKeys = (medthodName, params) => {
// add logic to get the right shopperClass here
return [
....Shopper*[medthodName],
...getCustomKeys(params)
]
}
There was a problem hiding this comment.
Good idea @alexvuong 👍
There was a problem hiding this comment.
@alexvuong please take a look at the latest changes. Inspired by your idea, I took it further and created a util method that picks the valid parameters. That's what the param keys are originally used for: to pick the valid parameters.
| const parameters = pickValidParams(netOptions.parameters, ShopperStores.paramKeys[methodName]) | ||
| const queryKey = queryKeyHelpers[methodName].queryKey(netOptions.parameters) | ||
| // We don't use `netOptions` here because we manipulate the options in `useQuery`. | ||
| const method = async (options: Options) => await client[methodName](options) | ||
|
|
||
| // For some reason, if we don't explicitly set these generic parameters, the inferred type for | ||
| // `Data` sometimes, but not always, includes `Response`, which is incorrect. I don't know why. | ||
| return useQuery<Client, Options, Data>(netOptions, queryOptions, { | ||
| return useQuery<Client, Options, Data>({...netOptions, parameters}, queryOptions, { |
There was a problem hiding this comment.
This change is different than the other ones. Because in this case, we forgot to filter out the invalid parameters, which is what the other files have done.
There was a problem hiding this comment.
Aside: this is another reason for having templates to generate the code. With it, we can avoid inconsistency/bug like this.
| @@ -40,7 +40,7 @@ | |||
| "version": "node ./scripts/version.js" | |||
| }, | |||
| "dependencies": { | |||
| "commerce-sdk-isomorphic": "^2.0.0", | |||
| "commerce-sdk-isomorphic": "^2.1.0-dev.0", | |||
There was a problem hiding this comment.
In another PR, I'll update this to v2.1.0.
This PR aims to refactor commerce-sdk-react code such that it no longer hardcodes the list of parameter keys. It will use the exported parameter keys from the underlying commerce-sdk-isomorphic instead.
For early review/feedback, I've made changes to only the ShopperProducts and ShopperBaskets:
ShopperBaskets.paramKeys['getBasket'])Todos:
Types of Changes
Changes
How to Test-Drive This PR
If you want to test drive it now, you'll need to build and symlink the isomorphic lib. Follow the instruction here, since it's still mostly relevant: SalesforceCommerceCloud/commerce-sdk-isomorphic#158You no longer need to symlink the isomorphic lib. You can now run
npm ciat the root, before you test-drive the PR.Since this PR is essentially a refactoring, we can test-drive it by smoke testing the retail-react-app site.
We can be confident in the code changes because of a few layers in place:
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization