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

Automatic ID generation does not work well with server rendering #1761

Closed
lemonmade opened this issue Jul 2, 2019 · 10 comments
Closed

Automatic ID generation does not work well with server rendering #1761

lemonmade opened this issue Jul 2, 2019 · 10 comments

Comments

@lemonmade
Copy link
Member

lemonmade commented Jul 2, 2019

Issue summary

In the latest edition of "mistakes Chris made several years ago", it's been bugging me that the approach to doing automatic ID generation in Polaris doesn't work properly for server rendering. I think all cases that need an ID generate one using createUniqueIDFactory. This creates auto-incrementing IDs (they aren't guaranteed globally unique, but they're fine for our purposes). However, they rely on module-global variables, which means that each request a node server gets will increment those IDs, even though the client will always start them back at 0. This creates a mismatch between client and server markup (not a huge deal, but an annoying warning in dev at the very least).

My proposal for addressing this would be to allow passing some sort of idFactory in to Polaris via context. This idFactory could default to being module global (e.g., const IdContext = createContext(idFactory())), which preserves the 0-config behaviour. However, for our apps, we could provide a fresh idFactory for each server render, allowing it to remain consistent with the eventual client markup.

Two things that would be nice to see out of it:

  • Make it a shared package so we could use it in other React components
  • Make sure it can be reset; our server rendering approach relies on rendering the whole app multiple times, so we need the ability to reset the counter between each pass.
@BPScott
Copy link
Member

BPScott commented Jul 2, 2019

See also #1283

@ghost
Copy link

ghost commented Dec 29, 2019

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

@ghost ghost added the Icebox label Dec 29, 2019
@ghost ghost closed this as completed Jan 6, 2020
@BPScott BPScott reopened this Jan 6, 2020
@BPScott
Copy link
Member

BPScott commented Jan 6, 2020

This is still an issue.

@BPScott
Copy link
Member

BPScott commented Apr 17, 2020

I've been low-key working towards a solution for this. Here's a state of the world update

History:

  • Sometimes our usage of createUniqueIDFactory was faulty and it caused incrementing Ids for every render of a component instead of just on initial render
  • functional components didn't have a good way of doing unique Ids that remained consistent across multiple renders of that component in the client, never mind keeping consistent between client and server initial renders

Mitigation so far:

  • useUniqueId() is a hook created to ensure stable ids across rerenders on the client side. It supports specifying a custom id factory, however that is not exposed from the AppProvider.
  • Most components that used createUniqueIDFactory have been converted to be functional components that use useUniqueId

The final fix for client/server mismatches will be contained within useUniqueId().

Next steps:

  • Migrate components that use createUniqueIDFactory to be functional components that use useUniqueId. Ongoing effort for this is tracked in Post v4 cleanup work #1995
  • Migrate Banner to be a functional component that uses useUniqueId (for some reason Banner has decided to go its own way and reimplement createUniqueIDFactory

Consider moving useUniqueId into @shopify/react-hooks

Update our ID handling by either:

  • Allow the id generator that useUniqueId uses to be specified in the AppProvider so that it may be resettable
  • Wait for a new version of react to ship with a stable version of useOpaqueIdentifier (see Add useOpaqueIdentifier Hook facebook/react#17322) that will automatically ensure consistent ids and update our useUniqueId to use that instead. The "provide a default and prefix" functionality will still be useful so keep them at least.

@Tom-Bonnike
Copy link
Contributor

Tom-Bonnike commented Aug 26, 2020

FYI you’ll be able to replace useUniqueId with an official react hook soon: reactjs/rfcs#32 (comment) Oh you mentioned that in the comment above. Nice.
I was bothered by the client/server mismatch for a while, I am refactoring the banner to a functional component using useUniqueId in #3199.

@ghost
Copy link

ghost commented Mar 16, 2021

This issue has been inactive for 180 days and labeled with Icebox. It will be closed in 7 days if there is no further activity.

@ghost ghost added the Icebox label Mar 16, 2021
@BPScott
Copy link
Member

BPScott commented Mar 17, 2021

Still a problem

@ghost ghost closed this as completed Mar 24, 2021
@BPScott BPScott reopened this Mar 24, 2021
@ghost ghost closed this as completed May 5, 2021
@BPScott
Copy link
Member

BPScott commented Aug 26, 2021

c'mon now probot

@BPScott BPScott reopened this Aug 26, 2021
@BPScott
Copy link
Member

BPScott commented Mar 8, 2022

React 18 introduces useId which gives consistent IDs between client and server. Once we require that, we replace the guts of useUniqueId (which is better than nothing, but not bug-free).

@github-actions
Copy link
Contributor

github-actions bot commented Apr 7, 2023

This issue seems inactive. If it's still relevant, please add a comment saying so. Otherwise, take no action.
→ If there's no activity within a week, then a bot will automatically close this.
Thanks for helping to improve Shopify's design system and dev experience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants