bloomreach / spa-sdk

Apache License 2.0
15 stars 16 forks source link

Has BrPage become a client component in react-sdk? #11

Closed cosieLq closed 1 year ago

cosieLq commented 1 year ago

I'm reading the code changes here: https://github.com/bloomreach/spa-sdk/commit/f7d9159503cdb206657f7bdc9e99cbff625391fc#diff-fc9893cb5040ad9f394c14325e50d32dc7328f7b9bdf9fa121776fbe61f53da5

Since page is set as 'undefined' in the constructor() of BrPage, and it is initialized in the async componentDidMount(), has BrPage essentially become a client component with a useEffect() hook wherein page is initialized?

Related questions are:

  1. What is the meaning of passing 'page' as props to BrPage?
  2. Will BrPage be rendered if JavaScript has been disabled?
cosieLq commented 1 year ago

I created this issue because in our case (Next.js) we initialize the page in getServerSideProps() and then pass page as props to BrPage.

I see the new render() method of BrPage. It returns 'null' in the first render cycle because page is 'undefined'. I don't think an async componentDidMount() will be awaited where page in props is actually checked. Does it mean that without JavaScript, BrPage will stay as 'null'? How does it work with SEO?

About design choice, what is the meaning of not using the initialized page in the first render cycle?

cosieLq commented 1 year ago

If there's no other way around, is it an idea to export not only 'BrPageContext' but also 'BrMappingContext' and 'BrNode' so that the users can at least build sort of their own BrPage?

beetlerom commented 1 year ago

Hello @cosieLq!

To answer some of your questions:

What is the meaning of passing 'page' as props to BrPage? Page is an object constructed from the response of the Page Delivery Api, you can read more about that here: https://documentation.bloomreach.com/content/reference/pages-json-representation

The BrPage component owns the responsibility of making the context of the Page available to all child components.

Will BrPage be rendered if JavaScript has been disabled? It depends on how you build your SPA. You can build a CSR, SRR or Isomorphic app.

We recommend an Isomorphic approach where you send an already rendered Page to the client on first request, ensuring optimal first render time and SEO, with subsequent navigations to other pages being partial re-renders in the browser.

I see the new render() method of BrPage. It returns 'null' in the first render cycle because page is 'undefined'.

I am guessing it returns null since you are using getServerSideProps sync? It's hard to tell without seeing the code, there are many ways to implement this. Providing us with a code sample or a repo is invaluable so that we can understand what issue you are facing.

About design choice, what is the meaning of not using the initialized page in the first render cycle?

There is a configuration property called non blocking render mode. This is useful if you want to render something like a loading spinner for part of an HTML page if you are pulling data/content from multiple sources (one of them being BloomReach content).

cosieLq commented 1 year ago

@beetlerom Thanks a lot for your answers! Unfortunately, I cannot show the code without permission, but our code resembles your example code for Next.js very much. I have two more questions about your example code:

marcin-jaskulski-wttech commented 1 year ago

Hi, we also struggle with SSR issue using spa/react-sdk 19.0.1 (client app implemented in NextJS).

I reviewed the BrPage implementation and I think I've found the reason why SSR doesn't work.

This line was introduced in version 18.0.1 https://github.com/bloomreach/spa-sdk/blob/main/packages/react-sdk/src/page/BrPage.tsx#L63 state.page is set to undefined, thus no data is used to initially render the page. During SSR only constructor and render method are triggered. I can't see how it can possibly work.

I checked version 18.0.0 and SSR was working correctly, although I can see many errors in authoring (like the one reported in #10). The constructor implementation looked like this: https://github.com/bloomreach/spa-sdk/blob/spa-sdk-18.0.0/packages/react-sdk/src/page/BrPage.tsx#L61-L65

For us, it's a critical (maybe even blocker) issue.

beetlerom commented 1 year ago

@cosieLq

Is the example using an Isomorphic approach that you meant?

Yes, the Next.js example app is an Isomorphic app. You can see that by the fact that the first page render is always generated on the server and when you try to navigate to a different page, a new set of props is returned instead of a new HTML page.

Will Page in the example be rendered when JavaScript is disabled? Have you tested it?

Yes, Isomorphic means the first request is always rendered server side, meaning that regardless of JS being enabled or not the page will always be rendered. This is currently broken in v18+ for react, but you can see it work in v17.1.1 for example.

beetlerom commented 1 year ago

@marcin-jaskulski-wttech Is correct, we are aware of this issue and are working to fix it as we speak.

mikroware commented 1 year ago

Is there by any chance an ETA on when this will be fixed?

joerideg commented 1 year ago

Hi @mikroware , We had to wrap up the Vue3 SDK first but I'm working on this right now. We are reverting the changes we introduced to the async / sync behavior and finding different ways to optimize for the bundle size. My estimation is to release within a week if all passes review and Q&A. Thanks for your patience!

cosieLq commented 1 year ago

Hi @joerideg , Thank you for your updates!

joerideg commented 1 year ago

Hi, we just released 21.0.0. This reverts the async behavior we introduced in 18. SSR is now fully supported again 👍🏻