ory / elements

Ory Elements is a component library that makes building login, registration and account pages for Ory a breeze. Check out the components library on Chromatic https://www.chromatic.com/library?appId=63b58e306cfd32348fa48d50
https://ory.sh
Apache License 2.0
85 stars 44 forks source link

fix: useIdWithFallback for react 17 and lower #122

Closed jorgeepc closed 1 year ago

jorgeepc commented 1 year ago

This PR fixes a compile error in projects using React 17. The change introduced in #113 implements unique IDs for components assuming that the useId react hook will be available. The proposed fix changes the import format to exclude the useId import from the build.

Related Issue or Design Document

See conversation in #120

Screenshot 2023-08-04 at 13 24 24

Sample dependencies to reproduce the issue:

"dependencies": {
    "@ory/elements": "0.0.1-beta.7",
    "loader-utils": "3.2.1",
    "react": "17.0.2",
    "react-dom": "17.0.2",
    "react-scripts": "5.0.1"
  },
  "devDependencies": {
    "@types/react": "17.0.2",
    "@types/react-dom": "17.0.2",
    "typescript": "5.0.2"
  },

Checklist

Further comments

Benehiko commented 1 year ago

I don't think we should implement webpack workarounds when the problem is useId is only supported in React 18. I think we should try find an alternative to this hook and remove it entirely. This ensures we have support for React < 18.

Others also have the same issue as we do: https://github.com/roginfarrer/collapsed/issues/117

aeneasr commented 1 year ago

If this works with R17 and R18 we should merge it, and remove it once webpack or React fix the issue upstream.

If it doesn't work, replace useId with a random string generator (potentially using setState), which has the same effect.

LBBO commented 1 year ago

What about outsourcing the dirty work? @over-ui/use-id also uses a webpack workaround (https://github.com/over-ui/unstyled/blob/main/packages/useId/src/useId.ts; the .toString() serves the same purpose as my workaround and is much nicer IMO) with a similar fallback to ours. Or should we rather keep it under our control so we can decide when to change back?

LBBO commented 1 year ago

After a brief chat with @aeneasr, we agreed to just keep it in our code. I switched to the other workaround anyways since it has better runtime. This version has been tested without npm link (I manually copied the build output into node_modules) in React 17 & 18 projects and work as expected.

jorgeepc commented 1 year ago

Hey @LBBO thank you for the priority with this. I tested the new release 0.0.1-beta.9 and it's working as expected. 🙌