Skip to content
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

[commerce-sdk-react] SCAPI Custom endpoint support #1793

Merged
merged 25 commits into from
May 22, 2024

Conversation

kevinxh
Copy link
Collaborator

@kevinxh kevinxh commented May 16, 2024

Description

This PR introduces support for calling SCAPI custom endpoint. It upgrades commerce-sdk-isormorphic to the latest version v2. Two new hooks are introduced in this PR.

New hooks

  • useCustomQuery
  • useCustomMutation

Types of Changes

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Documentation update
  • Breaking change (could cause existing functionality to not work as expected)
  • Other changes (non-breaking changes that does not fit any of the above)

How to Test-Drive This PR

  • pull this branch
  • npm ci
  • cd ./packages/test-commerce-sdk-react && npm start
  • go to localhost:3000/custom-endpoint
  • check the custom query is successfully made with 200 and correct auth token

Checklists

General

  • Changes are covered by test cases
  • CHANGELOG.md updated with a short description of changes (not required for documentation updates)

Accessibility Compliance

You must check off all items in one of the follow two lists:

  • There are no changes to UI

or...

Localization

  • Changes include a UI text update in the Retail React App (which requires translation)

@kevinxh kevinxh requested a review from a team as a code owner May 16, 2024 21:05
@@ -16,7 +16,7 @@ describe('Shopper Orders hooks', () => {
const unimplemented = getUnimplementedEndpoints(ShopperOrders, queries, mutations)
// If this test fails: create a new query hook, add the endpoint to the mutations enum,
// or add it to the `expected` array with a comment explaining "TODO" or "never" (and why).
expect(unimplemented).toEqual([])
expect(unimplemented).toEqual(['guestOrderLookup'])
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new method came from commerce-sdk-isomorphic@2

Comment on lines 20 to 22
'invalidateCustomerAuth', // DEPRECATED, not included
'authorizeCustomer', // DEPRECATED, not included
'authorizeTrustedSystem', // DEPRECATED, not included
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deprecated method removed from commerce-sdk-isomorphic@2

Comment on lines +30 to +32
shortCode="kv7kzm78"
clientId="4afbc51f-6423-41c8-8b29-d7f2825b5bee"
organizationId="f_ecom_zzrf_006"
Copy link
Collaborator Author

@kevinxh kevinxh May 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for custom endpoint manual testing, we have zzrf_006 setup for that

@@ -208,3 +209,14 @@ export type CacheUpdateMatrix<Client extends ApiClient> = {
? CacheUpdateGetter<MergedOptions<Client, Argument<Client[Method]>>, Data>
: never
}

type CustomEndpointArg = Parameters<typeof helpers.callCustomEndpoint>[0]
type CustomEndpointArgClientConfig = Parameters<
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type CustomEndpointArgClientConfig = Parameters<
type CustomEndpointParam = Parameters<typeof helpers.callCustomEndpoint>[0]
type CustomEndpointClientConfig = CustomEndpointParam['clientConfig']

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry i'm not quite understanding this diff, are you suggesting to rename CustomEndpointArgClientConfig to CustomEndpointParam?

This type is a partial of the entire param just to extract the client config.

@@ -40,7 +40,7 @@
"version": "node ./scripts/version.js"
},
"dependencies": {
"commerce-sdk-isomorphic": "^1.13.1",
"commerce-sdk-isomorphic": "^2.0.0",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we inadvertently force a breaking on this project by pumping this dependency as we are exposing the api clients via the useCommerceApi hook?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes i think so

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'm bumping the major version for commerce-sdk-react too

Comment on lines 125 to 129
`/${apiOptions.options.customApiPathParameters?.apiName || 'apiName'}`,
`/${apiOptions.options.customApiPathParameters?.apiVersion || 'apiVersion'}`,
`/organizations`,
`/${apiOptions.options.customApiPathParameters?.organizationId || 'organizationId'}`,
`/${apiOptions.options.customApiPathParameters?.endpointPath || 'endpointPath'}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm understanding the default values used to fallback on here, e.g. the static value 'apiName', 'apiVersion', 'organizationId' and 'endpointPath'.

  1. If apiName isn't defined we probably already failed making the call.
  2. we'll probably have to have duplicate logic that defines what the apiVersion is.. alternatively we have it exported from the isomorphic lib.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think customApiPathParameters should be required, but it's optional at the moment, shall we fix that in commerce-sdk-isomorphic

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call. i added a guard to make sure customApiPathParameters is defined before calling the api. otherwise throw error

@@ -21,7 +21,7 @@
"bundlesize": [
{
"path": "build/main.js",
"maxSize": "43 kB"
"maxSize": "44 kB"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ass we get older we get fatter 😭

Comment on lines 16 to 21
CacheUpdateGetter,
ApiOptions,
ApiMethod,
ApiClient,
MergedOptions,
CustomEndpointArgClientConfigOptional
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Might be an opportunity to sort this alphabetically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good

shortCode: config.organizationId
},
proxy: config.proxy,
...clientConfig
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This answers my previous question. 👍

@kevinxh kevinxh requested review from bendvc and alexvuong May 21, 2024 22:24
bendvc
bendvc previously approved these changes May 22, 2024
alexvuong
alexvuong previously approved these changes May 22, 2024
@kevinxh kevinxh merged commit dcfae70 into develop May 22, 2024
28 checks passed
@kevinxh kevinxh deleted the custom-endpoint-support branch May 22, 2024 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants