search

vikejs / vike-react

🔨 React integration for Vike
https://vike.dev/vike-react
MIT License
87 stars 13 forks source link

`useId()` unusable with SSR #36

Closed lourot closed 7 months ago

lourot commented 7 months ago

Moved from https://github.com/vikejs/vike/discussions/1291

useId() is supposed to generate deterministic unique IDs. Rendering the app first on the server and then on the client should lead to the same unique ID.

However in vike-react, because we feed react with different apps on server (including <head>) and on client (without <head>), we end up with different IDs, and so with hydration mismatches.

See steps to reproduce.

FYI @AndrejNemec

brillout commented 7 months ago

Makes sense, I think it's a regression that got introduced by https://github.com/vikejs/vike-react/pull/33.

brillout commented 7 months ago

@AndrejNemec PR welcome in case that's something you'd be interested in. The change is quite easy. Let us know if you're busy; we'll quickly fix it ourselves then.

AndrejNemec commented 7 months ago

Thank you for considering me for the PR contribution. I'm currently involved in a project named "vike-painless-react", which brings several key enhancements to the table:

You can explore the project here: vike-painless-react For a practical implementation example, check out: vike-painless-react example

While I'm currently focused on developing this package, I see potential synergies with Vike. If integrating these features aligns with your vision, my package could serve as a useful reference or even as a direct addition to Vike. I'm open to discussions about possibly making this an official part of the Vike portfolio. Looking forward to hearing your thoughts! 😄🌟

NPM: https://www.npmjs.com/package/vike-painless-react

phonzammi commented 7 months ago

Hi @brillout , About this

Shouldn't we just stream the page in the page-view tag? not the whole html?

brillout commented 7 months ago

@phonzammi Yes, that’s exactly the issue

On Wed 29. Nov 2023 at 18:15, phonzammi @.***> wrote:

Hi @brillout https://github.com/brillout , About this https://github.com/vikejs/vike-react/blob/a4ee8aa81dcc42873da311f6ab5642b3e0df16bd/vike-react/renderer/onRenderHtml.tsx#L38C3-L56C1

Shouldn't we just stream the page in the page-view tag? not the whole html?

— Reply to this email directly, view it on GitHub https://github.com/vikejs/vike-react/issues/36#issuecomment-1832366693, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAHVQRRLAZEEQPRMXCI2RCTYG5UTXAVCNFSM6AAAAAA74EGZSWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZSGM3DMNRZGM . You are receiving this because you were mentioned.Message ID: @.***>

phonzammi commented 7 months ago

@brillout Great, I've make a PR, Please review it, I have a question in the code comment

lourot commented 7 months ago

I'm currently involved in a project named "vike-painless-react", which brings several key enhancements to the table:

@AndrejNemec It sounds like some of these enhancement should be made to vike-react (whose goal is also to be painless) and to the equivalent vike-vue and vike-solid packages. PRs welcome

lourot commented 7 months ago

Fixed in v0.3.5

brillout commented 7 months ago

@AndrejNemec

I had a look at vike-painless-react: there are a lot of things to like in there 👌. Some of it, if you're interested, can be extracted in re-usable Vike packages e.g. vike-react-i18next, see converstaion we had with Dani at https://github.com/vikejs/vike/issues/1263.

(I grabbed the npm name of relevant Vike packages such as vike-react-i18next; feel free to let me know your npm user name and I'll add you as owner.)

As for your <head> management, I've my own ideas about it. Let's see which one turns out to be better 🦾 😉

In general, composing stacks from re-usable Vike packages and monolithic Vike stacks such as vike-painless-react are both valid approaches. I'm curious to see which one of the two approaches will turn out to be the most useful for users 👀

AndrejNemec commented 7 months ago

@brillout

This proposal I've put forward is just an initial idea. My goal is to create a monorepo containing specific packages and then set up an npm organization @vike-painless-react/X to develop several "painless" packages for vikejs.

The packages I've been considering are:

If you like this vision, we can jointly design it into the vike-react package, potentially replacing the vike-painless-react package. Furthermore, I believe it would be more maintainable to turn vike-react into a monorepo and create an npm organization @vike-react, structuring packages similarly to my proposal for vike-painless-react.

Additionally, I have a question regarding head management. How do you plan to implement it? I have used code from react-helmet-async. Many people are interested in implementing heads as found in next.js, where anywhere in the project, one can set the title, meta, etc., and at the same time, the title,meta,etc.. is fully reactive and SSR-friendly. Setting heads using +config.ts/+Head.tsx... doesn't seem like the most appropriate solution when I need to modify heads according to the state of the application.

brillout commented 7 months ago

create an npm organization @vike-react,

That's a great idea. I just created four npm orgs: @vike-{react,vue,solid,svelte}.

If you like this vision, we can jointly design it into the vike-react package, potentially replacing the vike-painless-react package.

Yes 👍

regarding head management. How do you plan to implement it?

One major issue with title/description defined by components is that it contradicts streaming: when the HTML stream starts then it's too late for setting title/description (because the <head> content is already sent).

That's why I'm relunctant to allow users to let components define <head> values.

I'm thinking +title.js and +description.js is more future-proof.

As much as I'd love to have title/description defined by components, I don't think it's possible because of how browsers work.

brillout commented 7 months ago

Actually, I'm wrong: a workaround would be to push this after the stream ends:

<script>
document.title = 'someTitleSetByAnyComponent'
</script>

As for SEO/social crawlers I'd recommend to disable streaming. This is actually what react-streaming does: https://github.com/brillout/react-streaming#options.

As for DX I'm thinking of that:

import { useTitle } from 'vike-react/useTitle'
// Can be vike-react-query or vike-react-telefunc
import { useData } from 'vike-react-someDataFetchingLibrary'

function Product() {
  const data = useData(() => fetch('...'))
  const head = useHead()
  head.setTitle(data.product.name)
  // No effect if streaming (which is fine since streaming is disabled for SEO/social bots)
  head.setDescription(data.product.description)
}

@AndrejNemec Thoughts?

AndrejNemec commented 7 months ago

That's a great idea. I would definitely also create components that use the useHead hook, allowing the setting of the title, description, and other useful meta tags like og:X, twitter:Xthrough components like <HeadTitle/>, <HeadMeta/>, <HeadLink/>, etc.

Furthermore, it would be beneficial to implement a robust injection system for packages that will be outside of the vike-react repository. Currently, it's not properly resolved and there are quite a few shortcomings. If I want to register a plugin to vike-react, it's quite a pain.

I propose something like this:

Let's say in the application I want to register the module: @vike-react/head

In the application, a config would be created: +modules.ts

export default {
  'vike-react-head': {
    server: 'import:@vike-react/head/module/server...',
    client: 'import:@vike-react/head/module/client...'
  },
  'vike-react-query': { /* ... */ },
  // ...etc
}
brillout commented 7 months ago

I'm not sure I understand your issue: what's wrong with https://vike.dev/extends?

AndrejNemec commented 7 months ago

The problem with "https://vike.dev/extends" is that some important values for the injection of multiple packages requiring the same methods are getting overwritten. This requires modifying the vike-react package every time to add new configuration attributes for each additional module I want to integrate into vike-react. For instance, I can't modify onRenderHtml without overwriting the original onRenderHtml, etc. This could be useful for developers who want to create more advanced packages for vike-react.

brillout commented 7 months ago

The plan so far is this: https://github.com/vikejs/vike/issues/1283. Which is actually needed for seamlessly integrating vike-react-query with vike-react. But, if you think it isn't enough, then elaborate on any concrete issue you're having.