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

W-14867867 Fix invalid query params warnings and allow custom query #1655

Merged
merged 23 commits into from
Feb 13, 2024

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Feb 3, 2024

Description

With the change from isomorphic that allow passing custom query (c_${string}). We'd want the commerce-react-sdk also adopt the changes. Commerce-react-sdk hooks will automatically accept any custom query key since we reuse type from isomorphic. However, we need to adjust our hooks to allow custom query key in react-query

This PR

  1. Fixes invalid params warnings seen on the retail react app. This is partially because commerce-react-sdk passes in invalid params from client config into isomorphic funcs
  2. Allow custom query key to be present in query key in react query cache

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

  • Check out code
  • npm ci
  • Run retail app
  • Observe there is not more invalid params warning in the console from any page
  • add custom query in any hook calls. E.g product-detail page
const {
        data: product,
        isLoading: isProductLoading,
        isError: isProductError,
        error: productError
    } = useProduct(
        {
            parameters: {
                id: urlParams.get('pid') || productId,
                allImages: true,
                c_customQuery: [true, false] => add it here
            }
        },
        {
            // When shoppers select a different variant (and the app fetches the new data),
            // the old data is still rendered (and not the skeletons).
            keepPreviousData: true
        }
    )
  • Check the api calls, and see that custom query is present in payload. e.g

image

  • Check that custom query params is also present in react query cache. e.g
    image

  • Add an custom query that does not start with c_. You should see a warning in the console. E.g

image

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)

@alexvuong alexvuong changed the title W-14867867 Fix invalid query params warnings W-14867867 Fix invalid query params warnings and allow custom query Feb 5, 2024
@alexvuong alexvuong self-assigned this Feb 5, 2024
@alexvuong alexvuong marked this pull request as ready for review February 5, 2024 23:18
@alexvuong alexvuong requested a review from a team as a code owner February 5, 2024 23:18
@johnboxall
Copy link
Collaborator

What happens if an API we call into adds support for a new query param?

For example, in 24.3, Product Search will add support for perPricebook, allImages and allVariationProperties. Will we need to modify commerce-sdk-react to allow folks to pass these params?

@alexvuong
Copy link
Collaborator Author

alexvuong commented Feb 6, 2024

What happens if an API we call into adds support for a new query param?

For example, in 24.3, Product Search will add support for perPricebook, allImages and allVariationProperties. Will we need to modify commerce-sdk-react to allow folks to pass these params?

API call-wise, new query param will get passed into the API call if isomorphic accepts those params since comemrce-react-sdk just passed those params into isomorphic functions for SCAPI calls.

However, for caching mechanism, we still need to adjust the queryKey to have new query param since we are manually passing in the params for each hook like what is being done in this PR for c_xx query See here

Copy link
Collaborator

@joeluong-sfcc joeluong-sfcc left a comment

Choose a reason for hiding this comment

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

Confirmed manually that Invalid Parameter warnings no longer appear and that passing a custom query parameter like c_param in a SCAPI call in template-retail-react-app can be seen as part of the SCAPI request using dev tools

Also, once you get this merged in, please close #1453 🙏

packages/commerce-sdk-react/src/hooks/useQuery.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@bendvc bendvc left a comment

Choose a reason for hiding this comment

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

For the most part everything looks good. But I think there is one thing to change to make this just right. See the comment here.

joeluong-sfcc
joeluong-sfcc previously approved these changes Feb 9, 2024
bendvc
bendvc previously approved these changes Feb 12, 2024
bendvc
bendvc previously approved these changes Feb 13, 2024
@alexvuong alexvuong dismissed stale reviews from bendvc and joeluong-sfcc via ad60e28 February 13, 2024 17:40
@alexvuong alexvuong merged commit 305a0b9 into develop Feb 13, 2024
28 checks passed
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

4 participants