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

Implement shopper products #681

Merged
merged 33 commits into from
Aug 30, 2022
Merged

Conversation

alexvuong
Copy link
Collaborator

@alexvuong alexvuong commented Jul 27, 2022

Description

This PR implemented useShopperCategories, useShopperCategory,useShopperProduct, useShopperProduct

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

How to Test-Drive This PR

  • Pull the code
  • npm ci
  • cd to the test project, npm start
  • Before clicking on any links, please grab a token in your console and put it inside your Provider here
  • Reload and see no error in the console, which means you are logged in as guest
  • Navigate around to see the hooks in actions!

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 added the wip Work in process label Jul 27, 2022
@alexvuong alexvuong requested a review from a team as a code owner July 27, 2022 19:59
@alexvuong alexvuong self-assigned this Jul 27, 2022
@alexvuong alexvuong added ready for review PR is ready to be reviewed and removed wip Work in process labels Aug 5, 2022
@vmarta
Copy link
Contributor

vmarta commented Aug 9, 2022

If you're curious why the CI tests are failing, it's because we need to import regenerator-runtime package when testing async functions.

I saw it in my branch, and I fixed it by reusing the jest setup file from internal-lib-build. That file has imported the regenerator-runtime package:

import 'regenerator-runtime/runtime'

Hopefully I can merge my PR soon, so you can pull it into your branch.

Copy link
Contributor

@vmarta vmarta left a comment

Choose a reason for hiding this comment

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

Other than this 1 comment, the whole PR looks good to me. Feel free to ask for someone else's feedback too, in case I overlook something.

Comment on lines +36 to +38
if (!arg.ids) {
throw new Error('ids is required for useProducts')
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

what happens if you don't add this if statement?

Will commerce-sdk-isomorphic throw a similar error? Should we use that error? Because ultimately the schema is coming from the commerce-sdk-isomorphic parameters

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If we don't catch the error here, it will hit the network and returns an error, since we are using react-query, by default it will retry 3 times before returning the error. I'd it is best if we can catch the error here before it hits the network. Here is what looks like if we don't catch that ids first
image

/* Interop Constraints */
"isolatedModules": true /* Ensure that each file can be safely transpiled without relying on other imports. */,
// "allowSyntheticDefaultImports": true, /* Allow 'import x from y' when a module doesn't have a default export. */
"esModuleInterop": true /* Emit additional JavaScript to ease support for importing CommonJS modules. This enables `allowSyntheticDefaultImports` for type compatibility. */,
Copy link
Collaborator

@kevinxh kevinxh Aug 29, 2022

Choose a reason for hiding this comment

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

I don't think we need this esModuleInterop flag? right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we need that to be true to avoid the complain from Typescript. By default, it is set to false

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.

left some non-blocking comments.

We had a discussion today and we noticed the internal-lib-build build script is not working correctly, the typescript declaration files are not generated in published package. We agree to fix that in a separate PR.

@alexvuong alexvuong merged commit 29377d4 into develop Aug 30, 2022
@wjhsf wjhsf deleted the feat/implement-shopper-products branch March 17, 2023 16:27
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.

4 participants