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

Simplify hooks logic #959

Merged
merged 132 commits into from
Mar 1, 2023
Merged

Simplify hooks logic #959

merged 132 commits into from
Mar 1, 2023

Conversation

wjhsf
Copy link
Contributor

@wjhsf wjhsf commented Feb 3, 2023

(This is a replay of #954 onto develop)

Over time, the implementations of query and mutation hooks have gotten a bit convoluted, and support has been added for features that we no longer deem necessary. This PR aims to simplify the signatures of the hooks and clean up the internal logic used.

As a result, query hooks now follow a pattern similar to:

const {
  data // Basket only, returning Response is no longer supported
} = useBasket(
  apiOptions // passed through to commerce-sdk-isomorphic
  queryOptions // passed through to @tanstack/react-query
)

Mutation hooks have also been simplified:

const { mutate } = useShopperBasketsMutation('createBasket') // Only takes the mutation name, now
mutate(apiOptions) // passed through to commerce-sdk-isomorphic

Under the hood, I've moved as much logic as I could from the endpoint hooks into the useQuery and useMutation helpers, to reduce repetition.

I've also refactored the query update logic. That must be implemented manually for each endpoint, so I've moved it out of the mutation.ts file, which is generated, and into a new config.ts file (per API, i.e. peer of mutation.ts). I've moved a lot of things around between the various locations (updateCache, useMutation, and the per-API methods), to hopefully simplify the interface as much as possible, and I've DRYed up some repeated logic.

In addition to cleaning up / fixing types, I made two major changes to query keys / cache updates: First, query keys previously only used parameters set on the options, but parameters can also be set on the config. So I've updated the logic to merge config/options before generating query keys / cache lookups. Secondly, we previously had cache lookup logic that accepted a query key, and created a lookup function from that. I've refactored the code so that we provide the lookup functions directly, which we can build from composable helpers. This provides us much more flexibility.

This PR will have a lot of file changes, so to keep review a bit more focused, I opened a separate PR with a snapshot, #981.

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

Localization

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

@wjhsf wjhsf added the ready for review PR is ready to be reviewed label Feb 3, 2023
* A hook for `ShopperContexts#getShopperContext`.
* Gets the shopper's context based on the shopperJWT.
* Gets the shopper's context based on the shopperJWT. ******** This API is currently a work in progress, and not available to use yet. ********
Copy link
Collaborator

@kevinxh kevinxh Feb 27, 2023

Choose a reason for hiding this comment

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

The js comments like these are usually not updated often and certainly not in the process when we release API changes. I wonder if there is a better place for such notes. Just a nit worry that we might forget to remove it in the future.

Copy link
Collaborator

@kevinxh kevinxh left a comment

Choose a reason for hiding this comment

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

The PR looks good to me, is it the intention to move the changes in pwa-kit-dev to another PR?

@wjhsf wjhsf merged commit e266437 into develop Mar 1, 2023
@wjhsf wjhsf deleted the wjh/hooks-on-develop branch March 1, 2023 20:23
@wjhsf wjhsf mentioned this pull request Mar 1, 2023
12 tasks
wjhsf added a commit that referenced this pull request Mar 3, 2023
* Simplify hooks logic (#959)

* Merge hooks changes from wjh/simplify-hooks

* lint fixes

* Use correct type.

* Remove unnecessary file.

* Add support for updater in cache updates.

* Sort imports.

* Change `updater` generic from being the same for all to being per function.

* Clean up cache update matrix.

* Rekerjigger cache logic to see what we can do with it.

* Create options merge util.

* Use merged options in query hooks and simplify query keys.

* Simplify endMatches logic.

* Add predicate helper to match API config values.

* Fix temp test file.

* Use consistent order of generic parameters.

* Remove pointless line.

* Use consistent generic parameter names.

* Add comments.

* More comments.

* Remove test directory.

* Generate hooks for all APIs

* Update Shopper Baskets config from Test directory.

* Update Shopper Experience query hooks.

* Fix missed merged options type.

* Add Shopper Login hooks.

* Convert getGiftCertificate from mutation hook to query hook.

* Update Shopper Login hooks.

Change logoutCustomer from query to mutation.
Change useRetrieveCredQualityUserInfo to getCredQualityUserInfo.

* First pass at converting to new cache update matrix.

* Remove canceled Shopper Discovery Search API.

* Update login helpers.

* Fix auth types.

* Hopefully avoid caching getGiftCertificate.

* Rename shopper login helper to auth helper.

* Move auth helper to hooks root.

* Mark useAuthorizationHeader as internal.

* Rename useAuth to useAuthContext for clarity w/ useAuthHelper.

* Convert useAuthHelper to default export for consistency.

* Remove siteId from AuthData, as it is passed via config.

* Update comment with generated code.

* Move headers to last parameter because we rarely use it.

* Update cache update matrices to new interface.

* Refactore test project after api changes.

* Clean up cache update logic.

Hopefully makes it much easier to maintain.

* Update names for clarity.

* Revert test file to version from develop.

* Add reminder to revert.

* Remove unnecessary default export.

* Cache update TODOs should return functions, not throw.

* Allow user to override authorization header.

* Remove old TODO.

* Remove ability to set custom query key.

Cache update logic requires knowing what query keys are used,
allowing arbitrary keys makes it impossible to know.

* Replace getQueryKey callback with just declaring query key.

* Clean up types.

* Rename config.ts to cache.ts

* Restore type checking test files.

* Suppress react query error logs during tests.

* Strip unused parameters before generating query key.

* Fix tpe errors.

* Partially fix type errors.

* Update Shopper Baskets cache logic to use narrower parameters.

* Test that all endpoints have hooks.

* Create new mutation tests for refactored code.

* Add test to validate all mutations have cache update logic.

* Alphabetize cache updates.

* Simplify mutation tests a bit.

* Fix nock adding duplicate request mocks.

* Make failing tests have more helpful failures.

Show the hook's error instead of "hook timed out" or nock mismatch.

* Fix tests failing due to missing parameters.

* Replace old mutation test file with new one.

* Remove unused helper type.

* Create hook success/error helpers.

* Improve parameter matching.

* Add `deleteBasket` tests.

* Add request mock to fix test.

* Add TODO cache update logic for missing mutations.

* Change onSuccess from bound to unbound function.

I'm not sure why, but testing mutations that throw an error in `onSuccess`
only works with unbound functions. When an unbound function is used, the
error is properly reported in the hook result. With a bound function, the
test framework throws an error.

* Add test for not implemented mutations.

* Pass not implemented tests.

* Remove unused import.

* Implement `resetPassword` caching as a no-op.

* Add Shopper Baskets query tests.

* Add "all endpoints have hooks" tests for all APIs.

* Fix failing index tests.

* Implement tests for all query hooks.

* Remove hooks for `authorizeCustomer` and `getTrustedAgentAuthorizationToken`.

These endpoints modify headers, rather than mutate or return data, so they
don't make sense for either query or mutation hooks. (At least, for our
current implementation.)

* Update "not implemented" tests to check if cache update logic exists.

* Extract reused type into type def.

* Update comment to reflect changed tests.

* Remove unnecessary `async`.

* Implement Shopper Contexts mutation tests.

* Rename `makeOptions` to `createOptions`.

* Implement Shopper Login mutation tests.

* Add extra ResponseError assertion.

* Update test names.

* Implement Shopper Orders mutation tests.

* Convert TODO from throwing to just logging.

* Fix failing "all endpoints have hooks" tests.

* Remove unused imports.

* Introduce query key helpers.

* Remove no longer needed query test file.

All query hooks are now ttested in their respective folders.

* Update hook usage.

* Implement basic Shopper Customers mutation tests.

* Export query key types.

* Remove old file.

* Update cache logic to use query keys instead of predicates.

* Update parser for proper TypeScript support.

babel-eslint is deprecated and superseded by @babel/eslint-parser.

* Remove unused babel-eslint.

* Temporarily(?) use internal-lib-build so TypeScript files are properly linted.

* Fix eslint and tsc errors.

* Temporarily suppress linter errors.

* Implement tests for shopper customers mutations that modify a customer.

* Implement full Shopper Customers mutation tests.

* Rename `StringIndexNever` to `StringIndexToNever`

* Remove unnecessary invalidation.

The query is already updated, so doesn't need to be invalidated.

* Update generated comments for hooks.

* Remove unused variable.

* Remove SLAS `authenticateCustomer` and `getPasswordResetToken` hooks.

They modify state and return headers, rather than body, so they are not
suitable for the current state of mutation hooks.

* Remove deprecated Shopper Customers mutations.

* Comment out Shopper Customers endpoints in closed beta.

* Fix Shopper Customers tests.

* Fix Shopper Login tests.

* Update "hook exists" tests to distinguish between "mutation in enum" and "mutation has cache logic"

---------

Co-authored-by: Ben Chypak <[email protected]>

* Fix package-lock.json

* Rename Shopper Login helpers to Auth helpers.

* Update to new hooks signature.

* Restore missing component.

How the heck did it get deleted?

* `enabled` must be a boolean.

* Restore missing imports.

* Implement `updateCustomerPassword` as no-op.

* Restore missing export.

* Restore missing prop types.

* Restore missing template names.

* Temporarily bump bundle size limit.

Re-visit after hooks work is completed.

* Update merged changes to use new format.

---------

Co-authored-by: Will Harney <[email protected]>
Co-authored-by: Ben Chypak <[email protected]>
Co-authored-by: Will Harney <[email protected]>
bfeister added a commit that referenced this pull request Mar 9, 2023
* develop:
  Allow query hook parameters to be `null`. (#1046)
  Implement `updateCustomerPassword` as no-op. (#1031)
  Update Retail React App Page Designer integration README (#1041)
  BUG: Changed type of the phone number field to bring up numberic keyboard on mobile devices - W-9871940 (#1016)
  Move the MRT reference app to the SDKs, so that we can verify eg. Node support (#966)
  Update `develop` with `release-v2.7.0` (#1033)
  Remove unused dependencies. (#1020)
  Make some style decisions on padding (#1023)
  Update CHANGELOG.md (#1021)
  Support product-sets in retail-react-app (#1019)
  Simplify hooks logic (#959)
  Fix NPM dependencies (#1012)

# Conflicts:
#	lerna.json
#	package-lock.json
#	package.json
#	packages/commerce-sdk-react/CHANGELOG.md
#	packages/commerce-sdk-react/package-lock.json
#	packages/commerce-sdk-react/package.json
#	packages/internal-lib-build/package-lock.json
#	packages/internal-lib-build/package.json
#	packages/pwa-kit-create-app/package-lock.json
#	packages/pwa-kit-create-app/package.json
#	packages/pwa-kit-dev/CHANGELOG.md
#	packages/pwa-kit-dev/package-lock.json
#	packages/pwa-kit-dev/package.json
#	packages/pwa-kit-dev/src/configs/webpack/config.js
#	packages/pwa-kit-dev/src/ssr/server/build-dev-server.js
#	packages/pwa-kit-react-sdk/CHANGELOG.md
#	packages/pwa-kit-react-sdk/package-lock.json
#	packages/pwa-kit-react-sdk/package.json
#	packages/pwa-kit-runtime/CHANGELOG.md
#	packages/pwa-kit-runtime/package-lock.json
#	packages/pwa-kit-runtime/package.json
#	packages/template-express-minimal/package-lock.json
#	packages/template-express-minimal/package.json
#	packages/template-retail-react-app/app/hooks/use-add-to-cart-modal.js
#	packages/template-retail-react-app/app/partials/product-view/index.jsx
#	packages/template-retail-react-app/package-lock.json
#	packages/template-retail-react-app/package.json
#	packages/template-typescript-minimal/package-lock.json
#	packages/template-typescript-minimal/package.json
#	packages/test-commerce-sdk-react/package-lock.json
#	packages/test-commerce-sdk-react/package.json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review PR is ready to be reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants