Wjh/simplify hooks #954

wjhsf · 2023-01-31T21:48:13Z

This PR is a draft of the changes to simplify the SCAPI hooks. The two primary goals are to fix the function signature (current develop has duplicate arguments and unnecessary complexity) and to move as much logic as possible from the endpoint hooks (which are generated) into the helper useQuery/useMutation hooks.

Very much still a WIP, but early feedback is appreciated! Currently only the query hooks are done, I haven't touched mutation hooks yet.

To avoid confusing myself during development, I've generated the ShopperBaskets hooks in a new Test directory. Once the design is more finalized, I'll remove Test and generate the real hooks.

Description

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)

Breaking changes include:

Removing a public function or component or prop

Adding a required argument to a function

Changing the data type of a function parameter or return value

Adding a new peer dependency to package.json

Changes

(change1)

How to Test-Drive This PR

(step1)

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...

Changes were tested with a Screen Reader (iOS VoiceOver or Android Talkback) and had no issues
Changes comply with WCAG 2.0 guidelines levels A and AA
Changes to common UI patterns and interactions comply with WAI-ARIA best practices

Localization

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

…meters.

Notably, this allows the correct type of the query key to be passed through to the callback function.

wjhsf

Ignore the test files (inconsequential changes) and the mutation/action files for now (not yet completed).

wjhsf · 2023-01-31T21:50:48Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

+export const useBasket = (
+    apiOptions: Argument<Client['getBasket']>,
+    queryOptions: UseQueryOptions<DataType<Client['getBasket']>>
+): UseQueryResult<DataType<Client['getBasket']>> => {


Design goal: Simple function signature - just pass through apiOptions to commerce-sdk-isomorphic and queryOptions to @tanstack/react-query.

wjhsf · 2023-01-31T21:54:32Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

+    const requiredParameters = ['organizationId', 'basketId', 'siteId'] as const
+    const queryKey = ['TODO']
+    const {shopperBaskets: client} = useCommerceApi()
+    const method = client.getBasket.bind(client)
+
+    return useQuery(
+        apiOptions,
+        {
+            queryKey,
+            ...queryOptions
+        },
+        {
+            client,
+            method,
+            requiredParameters
+        }
+    )


Design goal: Maximum DRY by only declaring endpoint-specific variables in one spot inside the function, and putting all the logic in the helper hook.

wjhsf · 2023-01-31T21:56:23Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

+    queryOptions: UseQueryOptions<DataType<Client['getBasket']>>
+): UseQueryResult<DataType<Client['getBasket']>> => {
+    const requiredParameters = ['organizationId', 'basketId', 'siteId'] as const
+    const queryKey = ['TODO']


I'm not planning on changing the query keys from what they currently are (for now), but there's currently no logic in the SDK generation tools to replicate them. (It won't be hard to add, but that's a tomorrow task.)

wjhsf · 2023-01-31T21:56:47Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

@@ -0,0 +1,158 @@
+/*


This file is automatically generated from the RAML spec!

wjhsf · 2023-01-31T21:57:25Z

packages/commerce-sdk-react/src/hooks/Test/action.ts

@@ -0,0 +1,364 @@
+/*


Ignore this for now. I haven't updated it since July, so it's very out of date!

wjhsf · 2023-01-31T22:00:41Z

packages/commerce-sdk-react/src/hooks/types.ts

+ * Adds additional parameters to a function signature. To preserve named parameters
+ * in the returned type, pass a named tuple for the extra parameters.
 */
-export type IMutationFunction<TData = unknown, TVariables = unknown> = (
-    variables: TVariables,
-    apiClients: ApiClients
-) => Promise<TData>
-
-/**
- * Modified version of React Query's Query Function. Added a second argument
- * API clients.
- */
-export type IQueryFunction<TData = unknown> = (
-    context: QueryFunctionContext<QueryKey>,
-    apiClients: ApiClients
-) => Promise<TData>
+export type AddParameters<
+    // eslint-disable-next-line @typescript-eslint/no-explicit-any
+    Fn extends (...args: any[]) => unknown,
+    Extra extends unknown[]
+    // TODO: Remove this after merging in prettier v2 changes
+    // eslint-disable-next-line prettier/prettier
+    > = (...newArgs: [...originalArgs: Parameters<Fn>, ...extra: Extra]) => ReturnType<Fn>


Created this for one iteration, evidently forgot to remove it.

wjhsf · 2023-01-31T22:01:20Z

packages/commerce-sdk-react/src/hooks/useAuthorizationHeader.ts

+import useAuth from './useAuth'
+
+// This feels like a bad name
+export const useAuthorizationHeader = <Arg extends {headers?: Record<string, string>}, Ret>(


Basically what useAuthenticatedClient does, but sets the authorization header on the method headers, rather than the config headers.

wjhsf · 2023-01-31T22:01:59Z

packages/commerce-sdk-react/src/hooks/useMutation.ts

+    [apiClients: ApiClients]
+>
+
+export const useMutation = <Data, Err, Vars>(


This was my first pass at cleaning up the existing types. I'll likely end up changing this a bit, as I did with useQuery.

wjhsf · 2023-01-31T22:03:52Z

packages/commerce-sdk-react/src/hooks/useQuery.ts

+    apiOptions: ApiArg,
+    queryOptions: UseQueryOptions<Data, Err, Data, QK> & {queryKey: QK},
+    hookConfig: {
+        client: Client
+        method: (arg: ApiArg) => Promise<Data>
+        requiredParameters: ReadonlyArray<keyof ApiArg['parameters']>
+        enabled?: boolean
+    }


Design goal: Mimic the format of the generated hooks. The third parameter is the variables we pass through from the generated hook in order to minimize repetitive logic in them.

wjhsf · 2023-01-31T22:06:33Z

packages/commerce-sdk-react/src/hooks/useQuery.ts

+        client: Client
+        method: (arg: ApiArg) => Promise<Data>
+        requiredParameters: ReadonlyArray<keyof ApiArg['parameters']>
+        enabled?: boolean


Intended to be used for extra "default" enabled checks, in addition to the "has all required parameters" logic, so that we don't have to duplicate the required parameter check outside this hook. For example, useCustomer might have enabled: customerType === 'registered'. The end user can still fully override using queryOptions.enabled.

bfeister · 2023-01-31T22:57:31Z

packages/commerce-sdk-react/src/hooks/ShopperBaskets/mutation.test.ts


-const tests = (Object.keys(mutationPayloads) as ShopperBasketsMutationType[]).map(
+const tests = (Object.keys(mutationPayloads) as Array<keyof typeof mutationPayloads>).map(


bfeister · 2023-01-31T23:00:42Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

+    return useQuery(
+        apiOptions,
+        {
+            queryKey,
+            ...queryOptions
+        },
+        {
+            client,
+            method,
+            requiredParameters
+        }
+    )
+}
+/**
+ * A hook for `ShopperBaskets#getPriceBooksForBasket`.
+ * Gets applicable price books for an existing basket.
+ * @see {@link https://developer.salesforce.com/docs/commerce/commerce-api/references/shopper-baskets?meta=getPriceBooksForBasket} for more information about the API endpoint.
+ * @see {@link https://salesforcecommercecloud.github.io/commerce-sdk-isomorphic/classes/shopperbaskets.shopperbaskets-1.html#getpricebooksforbasket} for more information on the parameters and returned data type.
+ * @returns An object describing the state of the request.
+ */
+export const usePriceBooksForBasket = (
+    apiOptions: Argument<Client['getPriceBooksForBasket']>,
+    queryOptions: UseQueryOptions<DataType<Client['getPriceBooksForBasket']>>
+): UseQueryResult<DataType<Client['getPriceBooksForBasket']>> => {
+    const requiredParameters = ['organizationId', 'basketId', 'siteId'] as const
+    const queryKey = ['TODO']
+    const {shopperBaskets: client} = useCommerceApi()
+    const method = client.getPriceBooksForBasket.bind(client)
+
+    return useQuery(
+        apiOptions,
+        {
+            queryKey,
+            ...queryOptions
+        },
+        {
+            client,
+            method,
+            requiredParameters
+        }
+    )
+}
+/**
+ * A hook for `ShopperBaskets#getShippingMethodsForShipment`.
+ * Gets the applicable shipping methods for a certain shipment of a basket.
+ * @see {@link https://developer.salesforce.com/docs/commerce/commerce-api/references/shopper-baskets?meta=getShippingMethodsForShipment} for more information about the API endpoint.
+ * @see {@link https://salesforcecommercecloud.github.io/commerce-sdk-isomorphic/classes/shopperbaskets.shopperbaskets-1.html#getshippingmethodsforshipment} for more information on the parameters and returned data type.
+ * @returns An object describing the state of the request.
+ */
+export const useShippingMethodsForShipment = (
+    apiOptions: Argument<Client['getShippingMethodsForShipment']>,
+    queryOptions: UseQueryOptions<DataType<Client['getShippingMethodsForShipment']>>
+): UseQueryResult<DataType<Client['getShippingMethodsForShipment']>> => {
+    const requiredParameters = ['organizationId', 'basketId', 'shipmentId', 'siteId'] as const
+    const queryKey = ['TODO']
+    const {shopperBaskets: client} = useCommerceApi()
+    const method = client.getShippingMethodsForShipment.bind(client)
+
+    return useQuery(
+        apiOptions,
+        {
+            queryKey,
+            ...queryOptions
+        },
+        {
+            client,
+            method,
+            requiredParameters
+        }
+    )
+}
+/**
+ * A hook for `ShopperBaskets#getTaxesFromBasket`.
+ * This method gives you the external taxation data set by the PUT taxes API. This endpoint can be called only if external taxation mode was used for basket creation. See POST /baskets for more information.
+ * @see {@link https://developer.salesforce.com/docs/commerce/commerce-api/references/shopper-baskets?meta=getTaxesFromBasket} for more information about the API endpoint.
+ * @see {@link https://salesforcecommercecloud.github.io/commerce-sdk-isomorphic/classes/shopperbaskets.shopperbaskets-1.html#gettaxesfrombasket} for more information on the parameters and returned data type.
+ * @returns An object describing the state of the request.
+ */
+export const useTaxesFromBasket = (
+    apiOptions: Argument<Client['getTaxesFromBasket']>,
+    queryOptions: UseQueryOptions<DataType<Client['getTaxesFromBasket']>>
+): UseQueryResult<DataType<Client['getTaxesFromBasket']>> => {
+    const requiredParameters = ['organizationId', 'basketId', 'siteId'] as const
+    const queryKey = ['TODO']
+    const {shopperBaskets: client} = useCommerceApi()
+    const method = client.getTaxesFromBasket.bind(client)
+
+    return useQuery(
+        apiOptions,
+        {
+            queryKey,
+            ...queryOptions
+        },
+        {
+            client,
+            method,
+            requiredParameters
+        }
+    )
+}


Question: this PR has a title simplify hooks and no description. Are we modifying only existing hooks or are we adding new ones?

When the PR title says "simplify" and the diff shows +658 that's confusing to me

Because it's a WIP that I threw up at 5PM before vanishing for a day. Figured it's better to have code with no context than no code at all.

It's a WIP. Note that the folder the new hook is in is Test; the actual hooks haven't been touched yet. I'm looking more for feedback on design than doing the actual code changes.

wjhsf · 2023-02-01T16:53:35Z

packages/commerce-sdk-react/src/hooks/Test/query.ts

+        [
+            'https://',
+            parameters.shortCode,
+            '.api.commercecloud.salesforce.com/checkout/shopper-baskets/',
+            parameters.version,
+            '/organizations/',
+            parameters.organizationId,
+            '/baskets/',
+            parameters.basketId,
+            '?',
+            'siteId',
+            parameters.siteId,
+            'locale',
+            parameters.locale,
+            // Full parameters last for easy lookup
+            parameters
+        ] as const


I used the full generated URL here as the query key. I can't imagine shortCode or version ever changing, so we probably don't need the base URI part. Definitely need path and query parameters. Not sure what the optimal format is, though.

alexvuong · 2023-02-01T23:56:01Z

This refactor should branch off the develop instead of the feature branch, the goal of the feature branch is to integration the hooks into the Retail App. Technically, it should not involve too much of a change from the commerce-sdk library. Besides, the feature branch is a long-run branch for this release, so I don't this we gonna merge it any time soon, and this refactor is likely to merge before the feature branch.

Making refactor from develop is also better because users can actually get to test out new code change when we do another release instead of having to wait until the feature branch is fully completed.

We can always merge this back into feature branch when it is done, it is easier than another way around

wjhsf added 8 commits January 24, 2023 16:22

Appease the linter.

4b7033e

Simplify types for useAuthenticatedClient.

f174ab0

Make useAuthenticatedClient accept a callback with any number of para…

08e0874

…meters.

Add helper type to add parameters to a function.

7188d21

Clean up useQuery types.

c59ecf3

Notably, this allows the correct type of the query key to be passed through to the callback function.

Clean up useMutation types.

1c46f4a

Argument should be non-nullable.

ea22c6b

Add first draft of query hooks refactor.

3fe4dbe

wjhsf commented Jan 31, 2023

View reviewed changes

bfeister reviewed Jan 31, 2023

View reviewed changes

Use generated URLs as query keys.

dd6ed04

wjhsf commented Feb 1, 2023

View reviewed changes

wjhsf added 7 commits February 3, 2023 13:14

Clean up type assertion.

ce0390c

Clean up mutations.

538e224

Fix types to remove Response from data

92b3aa2

Remove redundant Type

31ae612

Add temp file for easier validation that things work.

15e7dfe

Update comment and lint.

09b1a83

Generate Shopper Baskets hooks for diffing

073cf93

wjhsf mentioned this pull request Feb 3, 2023

Simplify hooks logic #959

Merged

12 tasks

wjhsf closed this Feb 6, 2023

wjhsf deleted the wjh/simplify-hooks branch March 15, 2023 22:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wjh/simplify hooks #954

Wjh/simplify hooks #954

wjhsf commented Jan 31, 2023 •

edited

Loading

wjhsf left a comment

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

wjhsf Jan 31, 2023

bfeister Jan 31, 2023

bfeister Jan 31, 2023

wjhsf Feb 1, 2023

wjhsf Feb 1, 2023

alexvuong commented Feb 1, 2023


		const tests = (Object.keys(mutationPayloads) as ShopperBasketsMutationType[]).map(
		const tests = (Object.keys(mutationPayloads) as Array<keyof typeof mutationPayloads>).map(

Wjh/simplify hooks #954

Wjh/simplify hooks #954

Conversation

wjhsf commented Jan 31, 2023 • edited Loading

Description

Types of Changes

Changes

How to Test-Drive This PR

Checklists

General

Accessibility Compliance

Localization

wjhsf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexvuong commented Feb 1, 2023

wjhsf commented Jan 31, 2023 •

edited

Loading