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

SSR only works when template query is present #1813

Closed
1 task done
rrmesquita opened this issue Feb 22, 2024 · 25 comments · Fixed by #1939
Closed
1 task done

SSR only works when template query is present #1813

rrmesquita opened this issue Feb 22, 2024 · 25 comments · Fixed by #1939
Assignees
Labels
type: bug Issue that causes incorrect or unexpected behavior

Comments

@rrmesquita
Copy link

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.

  • Yes
@rrmesquita
Copy link
Author

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.

@jasonbahl
Copy link
Contributor

@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?

@rrmesquita
Copy link
Author

@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.

@jasonbahl
Copy link
Contributor

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?

@rrmesquita
Copy link
Author

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

@rrmesquita
Copy link
Author

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

@jasonbahl
Copy link
Contributor

Thanks! This is helpful information 🙏

@theodesp
Copy link
Member

theodesp commented Feb 22, 2024

@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?

@rrmesquita
Copy link
Author

rrmesquita commented Feb 22, 2024

@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.

@theodesp theodesp added the type: bug Issue that causes incorrect or unexpected behavior label Feb 22, 2024
@jasonbahl
Copy link
Contributor

@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

@rrmesquita
Copy link
Author

rrmesquita commented Feb 23, 2024

@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.

@theodesp
Copy link
Member

@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.

@rrmesquita
Copy link
Author

rrmesquita commented Feb 26, 2024

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.

@theodesp
Copy link
Member

cc @blakewilson

@rrmesquita
Copy link
Author

rrmesquita commented Feb 27, 2024

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
Copy link
Author

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

@blakewilson

@blakewilson
Copy link
Contributor

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.

@MajesticPagan
Copy link

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!

@rrmesquita
Copy link
Author

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

@MajesticPagan
Copy link

MajesticPagan commented Jul 2, 2024

@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.

@alex-reid
Copy link

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.

@jasonbahl
Copy link
Contributor

👋🏻 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
Copy link
Contributor

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
Copy link
Contributor

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

@jasonbahl jasonbahl self-assigned this Jul 26, 2024
@rrmesquita
Copy link
Author

@jasonbahl have you looked into this? #1813 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issue that causes incorrect or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants