[Chakra v3] Fix data fetching regression with the SSR rendering (@W-18822712@)

[vmarta]

In the chakra v3 branch, we've observed a data-fetching regression with the server rendering, where most of the queries were not being run. Only the queries in the very top level components (like App, AppConfig) would get run.

It turns out that react-ssr-prepass and Chakra's use of React 18's useInsertionEffect are not compatible. When prepass is walking through the component tree, it needs to ignore the useInsertionEffect.. similar to how it's already ignoring useEffect and useLayoutEffect.

As a workaround, we've set useInsertionEffect to a no-op from within our SDK.

There's also another similar issue where Chakra's Portal component tries to access document which does not exist on the server side. Now we've created our own safe version of Portal that runs only on the client side.

Remaining todos:

• add E2E test? I don't think there's such test yet for verifying the server-rendered page.

How to test-drive the PR

1. Navigate to http://localhost:3000/?__server_only
2. Verify that the Shop Products section with its product data/images appear on the page.
3. If yes, that means that the query to fetch data for that section was run successfully
4. Now navigate to other pages (PLP, PDP) and verify that the resulting pages are very similar to the normal/client side version.

If you want to go further, you can also verify that there is no more error caught during prepass.

1. Run npm run start:inspect and make sure that you see the correct server logs through your Chrome browser.
2. Add a debugger according to the screenshot below
3. Navigate to http://localhost:3000/?__server_only
4. Verify that there is no error caught.

[cc-prodsec]

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

[kevinxh]

Interesting fix! I'd imagine it's hard to find this out, good job!

I'm wondering if you want to move this line to near the ssr-prepass related code, for example in packages/pwa-kit-react-sdk/src/ssr/universal/components/with-react-query/index.js

[vmarta]

I was thinking that we needed to set it to no-op very early on, at the start of entry point. But I can try moving it to with-react-query file and see whether the time matters or not.

[unandyala]

Again, just trying to understand the behavior due to this change. So all the components that use useInsertionEffect will now have a slight delay (may be not noticable) for the css application?

[vmarta]

I've just tried moving the line to the with-react-query file, but the app errors out. So I'm leaving the code change as it is now, but I've added some comment near the prepass call.

[vmarta]

@unandyala there shouldn't be any delay nor any change in behaviour. The workaround we implemented here is only for the server side. So when useInsertionEffect is called on the client side, it should behave the same as before.

🤔 Is there something else I need to consider? Let me know please, thanks.

[bendvc]

I'm wondering if we want to be a little be more clear on this for discoverability purposes.

We can either say what you have but mention + "due to chakra v3 upgrade".

So we can say something like "Ensure client-side only API's are called on the server"

I'm also ok with leaving it as is.

[bendvc]

Instead of having this one off logic, should we create a component..

Also there is this new that might fit the bill to wrap the render portion of this render. Not that I really want to solidify the Island component, but it's there.

[unandyala]

Just trying to understand the behavior here - since the portal components are closed by default there shouldn't be any performance impact, correct?

[vmarta]

@unandyala Yeah I think there shouldn't be any performance impact. But what performance-specific scenario or metric were you thinking of?

[alexvuong]

@vmarta There are a few components that is using Portal in its render. Would this happen whenever Portal is used?

[vmarta]

@alexvuong yeah, I'm going to replace all usage of Portal with our own version, which will render only on the client side.

[bendvc]

Similar comment as above, but it would be nice to angle this in a positive light since "regression" almost implies we unintentionally broke something and released it and fixed it. Since we didn't release it we might be able to say "Account for rending components that use reacts useInsertionEffect hook on the server"

[vmarta]

To meet the test coverage, I decided to add tests for this performance util. Yes, it's not related to the PR. But there isn't much changes in the react-rendering.js file that's worthy for testing.