fix: SSR compatibility for textfield #195

imprashast · 2024-01-23T10:26:17Z

Before this change, the value of id from the useId hook would initialize with null on server and get a value on client which broke SSR hydration. See screenshot of before:

To fix this, we can use the useId hook from react which generates this value on server as well, hence no mismatch between server and client. See screenshot below:

Notice how the value was printed twice before and only once after. This was because the our homemade useId hook ran only clientside with the useEffect hook.

github-actions · 2024-01-23T10:27:09Z

PR Preview Action v1.4.6
Preview removed because the pull request was closed.
2024-01-24 11:06 UTC

BalbinaK · 2024-01-23T11:30:34Z

packages/textfield/src/component.tsx

+
+    // useId returns a string that includes colons (:), e.g., :r0:, :r1:, etc.
+    // This string is NOT supported in CSS selectors. Hence the replace.
+    const id = providedId ?? useId().replace(/:/g, '');


Not sure why the original useId util had to rely on useLayoutEffect, but would it make sense to move the React useId() hook to src/useId.ts and simplify the previous useId util? I'm mostly thinking about reusing generateId as prefix, which would ensure unique ids in the case where we have multiple React apps using multiple TextField components on one page. I found an article about that here: https://hetdesai03.medium.com/a-complete-guide-to-useid-hook-in-react-18-22119ecfd87f Wdyt?

I think that old useId solution exists from react 16 days. This hook was added in React 18.

Notice also our useId hook used useLayoutEffect which is a performance pitfall, but it was common to use during React 16 days.

I'd say, we should replace instances of our useId with React's useId but maybe not in this PR.

TL;DR: Delete src/useId.ts and import it from react

I see. But what about generateId prefix util that would increase uniqueness of the id? If React's useId only generates r0, r1 strings, the chance of multiple TextFields across multiple React apps having the same id is higher. That's why I was thinking that we could maybe prefix those ids with whatever generateId returns, the same way it was done in the utils.

Hmm, the docs says that react will always generate unique id's for each useId within the vdom. Do you think there might be a situation where this won't hold?

multiple TextFields across multiple React apps

Do you mean in case of podlets?

Do you mean in case of podlets?

Yes, I was thinking about that :) If there were multiple React apps on a layout/page, served as podlets, then we could theoretically have multiple text fields with the same id, e.g. r0.

Okay, so I have updated our useId hook and tested it with other components :)

Ok, we have now confirmed that on SSR TextFields, the id is something like "R0ckd", "R0chj", so 3 additional characters compared to ids generated client-side ("r0", "r1"). I think we can assume at this point that there's little risk of having multiple client-side-rendered React apps with multiple TextFields, which would resolve in multiple r0, r1 ids. As mentioned in another comment, I agree that the preferred way to deal with it might be to ask the devs to pass { identifierPrefix: 'react-some-unique-prefix' } option to the createRoot() call.

vsandvold · 2024-01-23T22:21:46Z

Hey, thanks for fixing this problem so quickly!

You're right about multiple React apps on the same page getting the same IDs, unless each app is assigned an unique prefix. The React doc mentions a build-in solution for this, which is kind of obscure and indirect:

Your solution would be much better if it automatically assigns an unique prefix, and just works.

Perhaps I'm misunderstanding something, but it looks like the generateId() function includes a random number in the ID. Wouldn't this number be different from every function call, and therefore also different on the server and client? I would not be surprised if you have already checked that this works for SSR, so I'm just asking to be sure :-)

imprashast · 2024-01-24T09:37:00Z

packages/textfield/stories/Textfield.stories.tsx

@@ -1,13 +1,21 @@
 import React from 'react';
 import { action } from '@storybook/addon-actions';
-import { TextField as TroikaTextField } from '../src';


Troika :aaaaaaaaaaaaaa:

imprashast · 2024-01-24T09:39:53Z

Hey, thanks for fixing this problem so quickly!

You're right about multiple React apps on the same page getting the same IDs, unless each app is assigned an unique prefix. The React doc mentions a build-in solution for this, which is kind of obscure and indirect:

https://react.dev/reference/react/useId#specifying-a-shared-prefix-for-all-generated-ids

https://react.dev/reference/react/useId#using-the-same-id-prefix-on-the-client-and-the-server

Your solution would be much better if it automatically assigns an unique prefix, and just works.

Perhaps I'm misunderstanding something, but it looks like the generateId() function includes a random number in the ID. Wouldn't this number be different from every function call, and therefore also different on the server and client? I would not be surprised if you have already checked that this works for SSR, so I'm just asking to be sure :-)

Good point, I tested in a remix app and it was actually breaking hydration. Although, the id's generated on a SSR app was different than just the client side app. Keeping the old logic by updating serverHandoffComplete in a useEffect seemed unnecessary at this point. So let's just use the simple useId hook and mention the use of identifierPrefix for client side rendered apps.

BalbinaK · 2024-01-24T09:53:26Z

packages/textfield/src/component.tsx

@@ -28,9 +28,8 @@ export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(
    } = props;

    activateI18n(enMessages, nbMessages, fiMessages);
-
+    


BalbinaK

💪

## [1.3.1-next.1](v1.3.0...v1.3.1-next.1) (2024-01-24) ### Bug Fixes * SSR compatibility for textfield ([#195](#195)) ([7779237](7779237))

## [1.3.1](v1.3.0...v1.3.1) (2024-01-25) ### Bug Fixes * SSR compatibility for textfield ([#195](#195)) ([7779237](7779237))

fix SSR compatibility for textfield

bf81b45

imprashast requested a review from a team January 23, 2024 10:26

imprashast self-assigned this Jan 23, 2024

BalbinaK reviewed Jan 23, 2024

View reviewed changes

Update the useId hook

052075a

Cleanup useId hook

98cfdaa

imprashast commented Jan 24, 2024

View reviewed changes

Still need generateId for combobox

410fec6

BalbinaK reviewed Jan 24, 2024

View reviewed changes

BalbinaK approved these changes Jan 24, 2024

View reviewed changes

imprashast merged commit 7779237 into next Jan 24, 2024
7 checks passed

imprashast deleted the fix-textfield-ssr branch January 24, 2024 11:06

github-actions bot pushed a commit that referenced this pull request Jan 24, 2024

chore(release): 1.3.1-next.1 [skip ci]

6f06c50

## [1.3.1-next.1](v1.3.0...v1.3.1-next.1) (2024-01-24) ### Bug Fixes * SSR compatibility for textfield ([#195](#195)) ([7779237](7779237))

github-actions bot pushed a commit that referenced this pull request Jan 25, 2024

chore(release): 1.3.1 [skip ci]

146c7d9

## [1.3.1](v1.3.0...v1.3.1) (2024-01-25) ### Bug Fixes * SSR compatibility for textfield ([#195](#195)) ([7779237](7779237))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: SSR compatibility for textfield #195

fix: SSR compatibility for textfield #195

imprashast commented Jan 23, 2024

github-actions bot commented Jan 23, 2024 •

edited

Loading

BalbinaK Jan 23, 2024

imprashast Jan 23, 2024

imprashast Jan 23, 2024

BalbinaK Jan 23, 2024

imprashast Jan 23, 2024 •

edited

Loading

BalbinaK Jan 23, 2024 •

edited

Loading

imprashast Jan 23, 2024

BalbinaK Jan 24, 2024

vsandvold commented Jan 23, 2024

imprashast Jan 24, 2024

imprashast commented Jan 24, 2024

BalbinaK Jan 24, 2024

BalbinaK left a comment

		@@ -28,9 +28,8 @@ export const TextField = forwardRef<HTMLInputElement, TextFieldProps>(
		} = props;

		activateI18n(enMessages, nbMessages, fiMessages);

fix: SSR compatibility for textfield #195

fix: SSR compatibility for textfield #195

Conversation

imprashast commented Jan 23, 2024

github-actions bot commented Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imprashast Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

BalbinaK Jan 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vsandvold commented Jan 23, 2024

Choose a reason for hiding this comment

imprashast commented Jan 24, 2024

Choose a reason for hiding this comment

BalbinaK left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 23, 2024 •

edited

Loading

imprashast Jan 23, 2024 •

edited

Loading

BalbinaK Jan 23, 2024 •

edited

Loading