faustjs icon indicating copy to clipboard operation
faustjs copied to clipboard

SSR only works when template query is present

Open rrmesquita opened this issue 1 year ago • 18 comments

Description

When a template component doesn't have a query property, the SSR for that page simply doesn't work.

It looks like a bug to me because it doesn't make sense – if a page doesn't require remote data, there's no reason not to generate it statically.

In my reproduction example, I added a query to fetch some unused piece of data, just to show that the sole presence of a query triggers the SSR.

Steps to reproduce

  1. Clone my reproduction repository
  2. Install dependencies, run the build then npm run start to start the server
  3. In the browser, navigate to /sample-page
  4. Right-click and "View Page Source"
  5. Search for "testing" and assert it is not there
  6. Stop the server, go to wp-templates/page.js, uncomment the template query and save the file
  7. Run the build and start the server again
  8. Update the View Source page
  9. Search for "testing" and assert it is there.

Additional context

Page Source screenshot:

Screenshot 2024-02-21 at 22 02 59

@faustwp/core Version

2.1.2

@faustwp/cli Version

2.0.0

FaustWP Plugin Version

Using hosted example

WordPress Version

Using hosted example

Additional environment details

Using hosted example

Please confirm that you have searched existing issues in the repo.

  • [X] Yes

rrmesquita avatar Feb 22 '24 01:02 rrmesquita

To give more context, I'm not trying to create a page without remote data, as I could simply create a Next.js page for this.

Actually, I noticed this when trying to add multiple queries to a template, as described in the RFC.

rrmesquita avatar Feb 22 '24 01:02 rrmesquita

@rrmesquita thanks for reporting this!

I'd be curious to hear more about your use case for using a wp-template file if you don't require data from WordPress?

jasonbahl avatar Feb 22 '24 05:02 jasonbahl

@jasonbahl I may have expressed myself poorly. Sorry about it.

I do require data from WordPress. Actually, my use case is exactly the one described in the RFC: multiple queries, one being the page data and the other being layout/global pieces of content.

But when trying out with multiple queries and seeing that the SSR was not working, I discovered the problem described in this issue.

Let me know if I can provide more details.

rrmesquita avatar Feb 22 '24 06:02 rrmesquita

To clarify, you have Component.query or Component.queries the SSR works, but if neither are there it doesn't work and an error or some sort of debug messaging would be expected?

jasonbahl avatar Feb 22 '24 06:02 jasonbahl

Not really. The SSR only works with Component.query, but not with Component.queries. As for debug message, I couldn't see any.

rrmesquita avatar Feb 22 '24 06:02 rrmesquita

I've updated my reproduction repo to include an example of multi queries, where the SSR also doesn't work.

rrmesquita avatar Feb 22 '24 06:02 rrmesquita

Thanks! This is helpful information 🙏

jasonbahl avatar Feb 22 '24 06:02 jasonbahl

@rrmesquita I tried to reproduce your example but it seems that everything works as expected

Production build with no Component.query Screenshot 2024-02-22 at 14 17 15

Production build with Component.query Screenshot 2024-02-22 at 14 25 09

Production build with Component.queries Screenshot 2024-02-22 at 14 18 02

is there something we miss?

theodesp avatar Feb 22 '24 14:02 theodesp

@theodesp your screenshots show you're looking at the DOM inspection, rather than the actual page source. By the time you're looking at the DOM, the page has already been parsed, and the JavaScript has already been executed. The DOM is the result of the JavaScript execution, not the source of the page.

To view page source, right-click anywhere on the page, and click "View Page Source", right above the inspect option.

rrmesquita avatar Feb 22 '24 14:02 rrmesquita

@theodesp I was able to reproduce following @rrmesquita steps and disabling JavaScript in my browser. I only see the content if JavaScript is enabled which means this is not doing a Server Side Render but rather a client side render

jasonbahl avatar Feb 23 '24 17:02 jasonbahl

@jasonbahl @theodesp do you have any ideas on where I can take a look to help investigate this? I'm beginning a mid-size project, and getting this to work would be very useful. Also, I'd appreciate it if you could help me set up a dev environment for this repo.

rrmesquita avatar Feb 23 '24 21:02 rrmesquita

@rrmesquita TBH the bug is not obvious inside the getWordPressProps since we are returning data in the props as expected. On the other hand, I would look into the FaustProvider code that updates the seedQueries https://github.com/wpengine/faustjs/blob/canary/packages/faustwp-core/src/components/FaustProvider.tsx#L47

It could be that the use of context and useEffect would break SSR.

theodesp avatar Feb 26 '24 11:02 theodesp

Good guess, but React.useEffect is never executed in SSR.

I've run some tests, and I think the problem lies in this condition. When a query property is available, isPreview equals to false, otherwise it equals to null.

That null return on line 296 is most certainly causing this issue.

rrmesquita avatar Feb 26 '24 16:02 rrmesquita

cc @blakewilson

theodesp avatar Feb 27 '24 11:02 theodesp

Two other things came on mind related to this variable:

const [isPreview, setIsPreview] = useState<boolean | null>(
  templateQueryDataProp ? false : null,
);
  • Why it is possible to be null, instead of true | false only?
  • Shouldn't __FAUST_QUERIES__ also be considered when defining its initial value?

rrmesquita avatar Feb 27 '24 22:02 rrmesquita

Any updates on this? It seems pretty severe as it deals with SEO – also, half the fix is already pointed out here. Thanks!

@blakewilson

rrmesquita avatar Apr 16 '24 21:04 rrmesquita

Hi @rrmesquita,

I'm no longer working on the Faust.js project. @theodesp or @ChrisWiegman should be able to provide you with some information on the status of this issue though.

blakewilson avatar Apr 16 '24 21:04 blakewilson

Hello! Is there any updates on this? I'm having the same issue as well, when using queries SSR doesn't work.

@rrmesquita Have you found an alternative to this? Thank you!

MajesticPagan avatar Jun 24 '24 18:06 MajesticPagan

@MajesticPagan I had to stick with using the default query system (template queries).

rrmesquita avatar Jul 02 '24 17:07 rrmesquita

@rrmesquita Sad to read that but thank you the feedback!

I hope the Faust.js team takes a look at this in the near future. It's really tiring having to change and repeat the same global query through all the files.

MajesticPagan avatar Jul 02 '24 17:07 MajesticPagan

It's really tiring having to change and repeat the same global query through all the files.

Agreed. It would also be nice to be able to leverage some of Apollo's caching so that it isn't necessary to say load data for a header and a footer on every page load etc.

alex-reid avatar Jul 11 '24 06:07 alex-reid

👋🏻 I'm experiencing this issue on acf.wpgraphql.com as well. I'm actively looking into this and hope to have a contribution to Faust in the near future.

jasonbahl avatar Jul 26 '24 17:07 jasonbahl

For my use case, this appears to be directly related to useFaustQuery which relies on Context Providers which only work in the browser. My plan, at least initially, is to update useFaustQuery to allow an additional 2nd parameter so the data can be retrieved from the static props instead of the Apollo Client.

i.e.

useFaustQuery( MY_QUERY ); would become -> useFaustQuery( MY_QUERY, { __FAUST_QUERIES__ } );

If the __FAUST_QUERIES__ are passed to useFaustQuery the data can be grabbed from that instead of the Apollo client. If not, it would fall back to the Apollo Client as it does today.

This would allow users to have a minimal upgrade path and remain backward compatible.

Another option would be to introduce a different function, i.e. getQueryResponse( MY_QUERY, { __FAUST_QUERIES__ } ) or something along those lines and user could move from useFaustQuery to the new method when ready.

A few ideas to explore, but ultimately what needs to happen is that the __FAUST_QUERIES__ that are passed to the template file need to be used to access the data vs. using the Apollo client.

jasonbahl avatar Jul 26 '24 18:07 jasonbahl

Looks like this is probably deeper than useFaustQuery actually. Still digging into this.

jasonbahl avatar Jul 26 '24 18:07 jasonbahl

@jasonbahl have you looked into this? https://github.com/wpengine/faustjs/issues/1813#issuecomment-1964550821

rrmesquita avatar Jul 27 '24 19:07 rrmesquita