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

Make commerce-sdk-react Work Server-Side 💪🏻 #734

Merged
merged 24 commits into from
Sep 29, 2022

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Sep 22, 2022

Description

This the server-side support for react-query being merged into the SDK in this #724. It was important that we prove that the commerce-sdk-react hooks work was compatible. In this PR I make the required changes to ensure that any hooks used via the commerce-sdk-react module are executed server side.

I have left comments on part of this PR that I would like feedback on. So please pay special attention to those.

Why didn't this work after imply updating the sdk?

The reason this didn't work, is the commerce-sdk-react provider was itself using a react query provider. This provider was in conflict with the one supplied by the SDK. This meant that any hooks that we used used the client in the first provider it could find in its component hierarchy. This was the one defined in the commerce-sdk-react. Unfortunately the SDK does not know anything about this one and isn't able to track and execute the queries made by the hooks within it. This is the reason that it was required to be removed.

Changes

  • Remove the interfering ReactQueryProvider and client that was being defined in the SCAPI hooks provider.
  • Updated the test application to use the new withReactQuery HoC from the SDK
  • Added a section to the commerce-sdk-react readme on how its used in and out of a PWA-Kit application.
  • Updated SDK to allow the withReactQuery to consume options for setting the clients configuration.
  • Added default client configuration to never retry on the server. <-- Need feedback on this.

How to Test-Drive This PR

  • Checkout this branch.
  • Run npm ci
  • Run npm start
  • Your browser will open, once it's loaded goto this url --> http://localhost:3000/products?mobify_server_only=1
  • You shouldn't see a loading screen as you previously did, but instead you'll see the JSON data for the products requested immediately.

General

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

TODOs

  • Fix failing tests in commerce-sdk-react

- Updated the readme
- Updated the commerce sdk react to not include a provider
- Fixed the test app
- Allow config setting for client in sdk provider
@bendvc bendvc requested a review from a team as a code owner September 22, 2022 18:29
@bendvc bendvc added the wip Work in process label Sep 22, 2022
@bendvc bendvc changed the title Make commerce-sdk-reack Work Server-Side 💪🏻 Make commerce-sdk-react Work Server-Side 💪🏻 Sep 22, 2022
@bendvc bendvc added ready for review PR is ready to be reviewed and removed wip Work in process labels Sep 26, 2022
vmarta added a commit that referenced this pull request Sep 27, 2022
Since we plan to remove the QueryClientProvider in our commerce hooks. See PR #734

export const withReactQuery = (Wrapped) => {
export const withReactQuery = (Wrapped, options = {}) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add some jsdoc comment here to document the HOC, since it's gonna be public facing.

windowSpy.mockRestore()
})

test('Renders correctly', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a duplicate test

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.

From my end, there are a few remaining minor comments that have not been resolved yet. But overall, this PR looks good to me now.

Comment on lines +35 to +40
queries: {
retry: !server // Only retry when on the client.
},
mutations: {
retry: !server // Only retry when on the client.
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Bad example, this means never retry on the server, infinite retry on the client.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adamraya yeah, let's fix this readme next week.

@bendvc bendvc merged commit f25ef92 into develop Sep 29, 2022
@bendvc bendvc deleted the server-side-sdk-hooks branch September 29, 2022 23:01
vmarta added a commit that referenced this pull request Oct 4, 2022
* 1st attempt at disabling retry when seeing validation error

* Make sure the retries would stop

* Add todos

* Add comment

* Some refactoring and typing

* Remove some guards

* Add comments

* Remove comments

* Demonstrate user error

* React Query's retries are now done only for 5xx server errors

* For easier debugging

* Revert change

Since we plan to remove the QueryClientProvider in our commerce hooks. See PR #734

* Disable retries

And set the option at the individual query level.

* Revert adding query options on individual queries

* Remove todo

We can't use ResponseError yet, since it's not exported by the isomorphic sdk.

* Create a new page to see the errors

* Test validation error
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.

3 participants