Make API clients injectable in commerce-sdk-react (CommerceApiProvider)#2519
Make API clients injectable in commerce-sdk-react (CommerceApiProvider)#2519
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
shethj
changed the title
shethj
changed the title
kevinxh
left a comment
kevinxh
left a comment
There was a problem hiding this comment.
Code LGTM, can we add some documentation for this feature in the README?
|
First of all, just wanted to say: thank you and great job on this detailed PR description. For me who isn't following the progress closely, I feel like I now have a better idea of what's going on. |
| }, | ||
| throwOnBadResponse: true, | ||
| fetchOptions | ||
| const _defaultTransformer: SDKClientTransformer<Record<string, any>> = (_, _$, options) => { |
There was a problem hiding this comment.
🤔 I see the first 2 arguments to this function are not being used. So do we really need them?
Besides, it doesn't look like there's a way for the end user to pass in their own transformer.
There was a problem hiding this comment.
Yeah v2 integration is in a separate PR.
I see the first 2 arguments to this function are not being used. So do we really need them?
We're not using those arguments now: those 2 are props and methodName but we might need them for some future change. I could convert the props to transformer to be an object with optional properties to make it clear.
Also, we're not accepting a transformer from the end user for now is because they already have props in CommerceApiProvider where they can pass in all the different headers and options and our default transformer and SDK initialization already accepts all that when creating an instance.
However, the transformSDKClient function is open to accept any transformer function so in the future if we come across a use-case to allow the customer to pass in a transformer we can easily do that by simply creating another options prop for commerceApi.
I haven't added that today because I don't see a direct use-case for that right now.
| export type SDKClientTransformer<T> = ( | ||
| params: T, | ||
| methodName: string, | ||
| options: any | ||
| ) => any | Promise<any> |
There was a problem hiding this comment.
From what I see in the default transformer, it looks like the return object is like options but with some overrides/additions. If that's the case, can we make the type clearer?
| export type SDKClientTransformer<T> = ( | |
| params: T, | |
| methodName: string, | |
| options: any | |
| ) => any | Promise<any> | |
| export type SDKClientTransformer<T> = ( | |
| params: T, | |
| methodName: string, | |
| options: SomeOptions | |
| ) => SomeOptions | Promise<SomeOptions> |
4 participants
Description
As a part of the Plugin SLAS to Hybrid Auth transition, we’d like to enable customers using PWA Kit v2.x to take advantage of Hybrid auth capabilities. This requires some changes to
CommerceApiProviderin@salesforce/commerce-sdk-reactand more changes to integrate commerce-sdk-react in PWA Kit v2.xThis PR implements a refactor to @salesforce/commerce-sdk-react to allow injecting API Clients from the parent project (V2 template in this case) that uses the @salesforce/commerce-sdk-react package as a dependency.
This allows us to continue using the SDK client setup in PWA Kit v2 and also allow customers to take advantage of the new react query hooks and auth improvements we've made in commerce-sdk-react.
Most important, this change paves a simple path for customers to be able to upgrade to future versions of PWA Kit where they can combine use of V2 CommerceAPI and V3's commerce-sdk-react to migrate their storefront components over gradually.
As a side-effect of this change, we also laid foundation and have a working POC for fully decoupling the SDK Client instantiation from commerce-sdk-react which allows us to move commerce-sdk-isomorphic as a dev-dependency of the react SDK and eventually boost performance by reducing bundle sizes via tree-shaking.
NOTE: Tree shaking improvements requires implementing ESM support for commerce-sdk-isomorphic.
Overall Goals for this work
To summarize the description above we want to achieve the following:
The items marked ✅ are the ones we want to merge for now while the items marked⚠️ involve breaking changes to the V3 retail template and also require work in the isomorphic-sdk.
This PR includes changes required to merge in items marked as ✅.⚠️ have already been implemented as a POC in a different PR but will be held off until PWA Kit v4 where we have capacity to include breaking changes. See this PR
This gives us significant improvements while avoiding any breaking changes.
The items marked
Types of Changes
Changes
apiClientsto CommerceApiProviderNote: Since we made
apiClientsinjectable and optional and customers can choose to instantiate only the SDK classes that they actually use, we need to handle cases where the template might call a query or a mutation hook for SDKs that have not been passed in. This change is in a follow-up PR. See #2539How to Test-Drive This PR
npm startfrom monorepo rootChecklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization