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

[Feature] Integrate react-query with PWA-Kit SDK #693

Closed
wants to merge 45 commits into from

Conversation

bendvc
Copy link
Collaborator

@bendvc bendvc commented Aug 8, 2022

Description

The objective of this PR is to integrate React Query with PWA-Kit, with the primary intention to allow use to build a hooks library that is compatible with our server-side rendering. Another motivating factor is to provide an alternative, not a replacement, to the aging getProps interface.

This PR takes advantage of a library called react-ssr-prepass to record all usages of react-query in the "to be rendered" application so we can resolve those query requests, serialize the returned data, and continue on with the rending flow.

The solution in this PR diverges slightly from the solution provided by @olibrook, in that we take special care to not serialize the awaiting of getProps and and react-query queries. In order to accomplish this I had to reorganize some of the code so that the prepass stage happens before the "appState" is initialized.

During the implementation I ran into a couple things I'd like some input on:

  1. Because the prepass stage have been moved outside of the renderApp method, if it was to throw, we would have an uncaught error. Although I did some tests to get the prepass stage to fail, it seems like because it only traverses the JSX elements it doesn't throw errors itself if a component was to throw in its render. I'm guessing it's probably safer to ensure we catch errors no matter what and pass them along to the renderApp method.
  2. I've tried various methods, whether its setting defaultOptions on the <Hydrate > component so that or options on the queryClient, to stop hydrated queries from being called again. But I haven't found what would be an acceptable solution, aside from setting the staleTime to a value that would cause the query to not be stale at the type of hydration. But that seems a little flaky. It would be nice to find a better solution for that.

_NOTE 1: The server side resolution of react-query queries is an opt-in feature, as the prepass state for rendering take proportional time to how long it takes to render. _

NOTE 2: Support for dependent queries as described here is currently NOT supported for server side rendering. Queries that are not enabled will not be run on the server.

TODO's

  • In order to provide a on-par solution to getProps we need to have access to the req/res objects. I'm thinking about creating a provider that will have the express request context. Maybe the name would be something like useExpressCallbackArgs. I'm open to name suggestions.
  • Write tests.
  • Handle prepass errors?
  • Come to conclusion on handling how we stop queries from being triggered during hydration.

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)

Changes

  • Added ssrPrepassEnabled feature flag. The reason for calling it ssrPrepassEnabled as opposed to something with react query or use query in it, is because you could still use useQuery client side, there would be no way to stop partners from doing that, so enabledUseQuery would be a misnomer.

How to Test-Drive This PR

  • Use the typescript minimal app. Its been set up to request data from a rando api endpoint that Oli set up. You can press the "Click Me" button to fetch a new Chuck Norris quote.

Checklists

General

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

@bendvc bendvc requested a review from a team as a code owner August 8, 2022 23:10
@bendvc bendvc requested review from olibrook and kevinxh August 8, 2022 23:11
@bendvc bendvc added the ready for review PR is ready to be reviewed label Aug 8, 2022
})
: Promise.resolve({})
)
const queries = queryClient.queryCache.queries.map((q) => q.fetch())
Copy link
Collaborator

@kevinxh kevinxh Aug 9, 2022

Choose a reason for hiding this comment

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

what happens when the query throw error? Is react-query able to hydrate the error?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There are some limitations with serialization of errors. I can play around with it to see the actual behaviour, because it's not completely clear what happens to errors if you tell dehydrate to serialize them. Since it says you can't do that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. From the documentation, it looks like react-query does not provide a serializer for errors.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It would be likely that the error would be handled one of a handful of ways that don't result in the need for serializing the error.

  1. The error is thrown and our server pipeline will do the appropriate things with it, e.g. set the status code, render the error page.
  2. The error is not thrown and handled on the page in which the page would not render an appropriate UI.

I still want to see what would happen is a query throws an error internally in its query function, then on the client we wouldn't re-run that query. I'm curious as to what data and or error are set to.


return (
<ExpressContext.Provider value={{req, res}}>
<QueryClientProvider client={queryClient}>
Copy link
Contributor

@vmarta vmarta Aug 9, 2022

Choose a reason for hiding this comment

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

Suggested change
<QueryClientProvider client={queryClient}>
<QueryClientProvider client={queryClient} contextSharing={true}>

In response to @kevinxh 's question on Slack recently: I think both pwa-kit-react-sdk and commerce-sdk-react packages can mount their own QueryClientProvider, that is, if commerce-sdk-react is meant to be a standalone library. By setting contextSharing to true, it looks like it's ok to have multiple QueryClientProvider in the component tree, and only one instance will be used.

What contextSharing option does: https://tanstack.com/query/v4/docs/reference/QueryClientProvider

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

commerce-sdk-react isn't going to be mounting it's own provider. That being said, it doesn't mean that someone wouldn't add their own for some weird reason. The question is, do we want to make that decision for the user.. also does context only get shared amongst providers that have that value set to true? hmm

bendvc added 3 commits August 10, 2022 15:51
- Make useQuery example in ts minimal server side only
routes
})

;({appState, error: appStateError} = await WrappedApp.fetchState({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Dumb question, why do we need to wrap this line around ;( ... ) ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This line isn't in the code anymore, but for education purposes. I found out that this is how you accomplish using let to declare variables and have them be assigned to during destructuring.

The reason I was having to do this is because appState and appStateError were declared outside of this conditional because they are being used elsewhere. Alternatively you could use an intermediate variable like this

const result = await WrappedApp.fetchState(...)
appState = result.appState
appStateError = result.appStateError

@vmarta
Copy link
Contributor

vmarta commented Aug 29, 2022

@bendvc Follow-up to our earlier conversation on Slack: yup, there is a bug that can be seen by loading the retail-react-app-template, specifically with how extraGetPropsArgs is being shared across components.

In the retail-react-app:

  • api is undefined in the App.getProps
  • BUT it is defined in page components like Home.getProps

{}
)
} catch (e) {
appStateError = logAndFormatError(e || new Error())
Copy link
Contributor

Choose a reason for hiding this comment

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

Querying Commerce API for a non-existent category, for example, would throw an error, which would get caught in this catch statement. The end result is this internal server error:

Google Chrome 2022-08-30 at 16 50 48

But what if the developers would want to catch and handle it in their app? Currently they can't because it's caught in the react rendering.

Copy link
Contributor

@vmarta vmarta Aug 30, 2022

Choose a reason for hiding this comment

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

One scenario I'm investigating is for the useExpress hook: how to detect a non-existent category and spit out a 404. I would want to be able to do the following in the user land:

    const {categoryId} = useParams()
    const {data, isLoading, error} = useCategory({id: categoryId, levels: 0})
    const {req, res: pageRes} = useExpress()

    if (pageRes && error?.response?.status === 404) {
        pageRes.status(404).send(data?.detail)
    }

We were able to do a similar thing in getProps:

// The `isomorphic-sdk` returns error objects when they occur, so we
// need to check the category type and throw if required.
if (category?.type?.endsWith('category-not-found')) {
throw new HTTPNotFound(category.detail)
}

const queries = queryCache.getAll()
const promises = queries
.filter(({options}) => options.enabled !== false)
.map((query) => query.fetch().catch((e) => ({error: e})))
Copy link
Contributor

Choose a reason for hiding this comment

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

In case users would want to use the other error-related properties that are returned by useQuery:

Suggested change
.map((query) => query.fetch().catch((e) => ({error: e})))
.map((query) => query.fetch().catch((e) => ({error: e, status: 'error', isError: true})))

Split useLegarcygetProps and routeComponent again
Move logic for setting preloadedProps into the HOC
Fix issue with useQuery throwing in render
Comment on lines +71 to +79
return {
error,
errorUpdatedAt: Date.now(),
errorUpdateCount: 1,
isError: true,
isLoadingError: true,
isRefetchError: false,
status: 'error'
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I found that this object is not what I see on the user land. The object has more things in it.. more complete.

Is this because although we're returning all promises on line 82 below, what gets saved is actually the queryClient itself on line 84?

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