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
84 stars 40 forks source link

fix: compile error useId only available in react18 #120

Closed jorgeepc closed 11 months ago

jorgeepc commented 11 months 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

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

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

Benehiko commented 11 months ago

Thank you for the contribution @jorgeepc, I will take a look when I get time this week. Unless @LBBO you want to take a look?

LBBO commented 11 months ago

Already on it 😉

LBBO commented 11 months ago

Hey @jorgeepc, I just tested the deployed version (@ory/elements@0.0.1-beta.8) with your changes and it turns out that my testing earlier was incorrect. I just built your branch and linked it into a test project, where the react import seems to have referred to the local react version instead of the host's react version. Is this how you tested your changes as well? Or did you test them differently?

Anyhow, using the same host project but the deployed version, an npm run build now fails. Could you please check if it also fails for your project? If not, could you perhaps share a bit more about the setup (especially the bundler you use)?

LBBO commented 11 months ago

Alright, it seems that we need to import useId without webpack realising it. For me the following code works:

const fallback = () => Math.random().toString(36).substring(2)

/**
 * A function to obtain a unique ID. If react is available, this
 * is just a wrapper for React.useId(), meaning the ID will be
 * consistent across SSR and CSR. Otherwise, it will be a random and
 * unique ID on every call.
 */
export const useIdWithFallback = () => {
  const useId =
    (Object.entries(React).find(([key]) => key === "useId")?.[1] as
      | undefined
      | (() => string)) ?? fallback
  return useId()
}

In order to test the general idea, you can install the latest deployed version and then manually edit the node_modules/@ory/elements/dist/index.mjs:

--- index_original.mjs   2023-08-08 16:44:21
+++ index_new.mjs      2023-08-08 16:44:14
@@ -1184,7 +1184,8 @@
   }
 ), useIdWithFallback = () => {
   try {
-    return require$$0.useId();
+    const useId = Object.entries(require$$0).find(([key]) => key === 'useId')[1];
+    return useId();
   } catch {
     return Math.random().toString(36).substring(2);
   }

After that, the build succeeds in my project.

LBBO commented 11 months ago

Also, just to share my research:

The error message is thrown by webpack: https://github.com/webpack/webpack/blob/4938e7715dce34b177fedb85d7f190e6e0a80b03/lib/dependencies/HarmonyImportSpecifierDependency.js#L250

It should be configurable via strictExportPresence (see https://webpack.js.org/configuration/module/#module-contexts), but changing webpack config is often a pain in the back and I'd prefer having a different fix (especially since this would force project-wide config changes).

jorgeepc commented 11 months ago

Hey @LBBO I tested the deployed version in my setup and you are right. I see it failing for the build. I think there was some webpack cache issue when I tested my original approach.

I validated your idea and it's working on my end. If you like I can open a new PR with the solution. Please let me know.

jorgeepc commented 11 months ago

@LBBO Here I have the PR in case you want to proceed with the fix https://github.com/ory/elements/pull/122