pwa-kit icon indicating copy to clipboard operation
pwa-kit copied to clipboard

[Feature] Integrate `react-query` with PWA-Kit SDK

Open bendvc opened this issue 2 years ago • 2 comments

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 avatar Aug 08 '22 23:08 bendvc

@bendvc I took a look at your HOC work. This is definitely the direction I was imagining. I spent some time tinkering on Friday and didn't quite get anything presentable. Big picture, I'd suggest some acceptance criteria for you first:

  1. Let's agree that we are fully replacing our recommendation to use getProps with a recommendation to use react-query for all pwa-kit data-fetching, moving forward. Design for it as the new default.
  2. Existing getProps users should not pay performance penalties for react-query if they're not using it (neither bundle size, nor SSR speed).
  3. We don't want to invent new config for this that we think we'll come to regret (eg. ssrOnly or the proposed ssrPrepass).

If we can agree on that, we can talk APIs! Fundamentally, I think we need to move the data-fetching parts out of the react-rendering code and into something that the user can configure. So, where we have this in render:

    // Step 3 - Init the app state
    const {appState, error: appStateError} = await initAppState({
        App: WrappedApp,
        component,
        match,
        route,
        req,
        res,
        location
    })

We should find a way to do this instead, handing responsibility to the App or a HOC to do the initAppState stuff:

    const {appState, error: appStateError} = await WrappedApp.initAppState({
        App: WrappedApp,
        component,
        match,
        route,
        req,
        res,
        location
    })

You could then find a way of making the WrappedApp contents swappable, and using an HOC sounds like a great way of doing it. This is nearly what you have, except I'm suggesting offload initAppState to the HOC in its entirety, not just getAllQueryPromises.

Once we've done that, I would strongly recommend that we make the react-query behaviour the default with no additional HOC at all in user-space. I would set it up in such a way that getProps users need to opt-out of the new default, by adding an HOC to an App component. We want the burden of config for people that are using react-query to be zero, if it's our recommended default.

So, I'd expect us to make a major version bump with a single breaking change: Existing getProps users that don't want to upgrade have to opt out of the new behaviour by doing something like this in user code:

export default useLegacyGetProps(App)

This is a breaking change, but very easy to do for users who don't want to upgrade their code. Perfect example of when I think we should do it!

olibrook avatar Aug 15 '22 21:08 olibrook

I like the idea of offloading the initAppState function. That makes it very easy to define what the API is for any of those HOC's. I'll play around with it and let you know if I have any questions.

UPDATE: As per our discussion on friday, we do want to ensure that react-query can in fact do everything getProps does if we are going to make it our recommended default in the major version bump. Unless you are recommending the API allow you to ultimately stack these data providing HOC's on top of each other, but I don't think you are as that would probably get quite complex pretty fast.

bendvc avatar Aug 15 '22 21:08 bendvc

@bendvc @johnboxall People are rightly worried about breaking changes. I think that the kind we should care about are those that have no justification and no benefit to the end user. Is this one of those occasions?

My answer is no. You haven't spelled it out, but I think you are going to re-write the retail-react template in the near future so that it barely uses getProps, if at all. When you do that, I think you're going to find that when users ask "how should I fetch data from [service XYZ] in my app?", you'll want to recommend use-query – a single answer, across the board.

As such, I think you should bias your design towards what you are going to be recommending users do by default soon.

If you do a major version bump then your upgrade guide is going to say something like this:

We've added support for react-query in this release. If you are happy using getProps and you want to opt-out of react-query support, wrap your App component with the useLegacyGetProps HOC – if you do this, you'll avoid doing an SSR prepass on the server-side, which will improve your performance.

This would be a very easy upgrade guide to follow and would be balanced against the – apparent! - benefit of switching to something hooks-based, which y'all clearly want to do...

olibrook avatar Aug 17 '22 19:08 olibrook

@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

vmarta avatar Aug 29 '22 23:08 vmarta