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

[base-ui] "useId" import error after updating esbuild #41190

Closed
patrykszwed opened this issue Feb 19, 2024 · 16 comments · Fixed by #43360
Closed

[base-ui] "useId" import error after updating esbuild #41190

patrykszwed opened this issue Feb 19, 2024 · 16 comments · Fixed by #43360
Assignees
Labels
bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it package: base-ui Specific to @mui/base ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@patrykszwed
Copy link

patrykszwed commented Feb 19, 2024

Steps to reproduce

Link to live example: CodeSandbox

Steps:

  1. Run npm run build.
  2. Open /lib/cjs/index.csj.js and /lib/esm/index.js files.
  3. Search for maybeReactUseId variable in both of them.

Current behavior

Note: this issue is similar to #37182, #37183 and #29860. Its root cause is probably related to that Webpack issue.

Observations

What can be observed in /lib/cjs/index.cjs.js file:

var maybeReactUseId = React6["useId".toString()];

What can be observed in /lib/esm/index.js file:

var maybeReactUseId = React6.useId;

That difference is caused by enabling minifySyntax: true in bundle.js only for the "esm" format. esbuild was not minifying ["useId".toString()] to .useId until version 0.18.20 (including). However, it's been doing this since 0.19.0 release (I've checked the latest esbuild version and the outcome is the same).

Scenario

Now let's say this package is an internal components library which is reused in another app that also uses React 17. When one tries to build that app using webpack an error will be thrown:

export 'useId' (imported as 'X') was not found in 'react' (possible exports: Children, Component, Fragment, Profiler, PureComponent, StrictMode, Suspense, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, cloneElement, createContext, createElement, createFactory, createRef, forwardRef, isValidElement, lazy, memo, useCallback, useContext, useDebugValue, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useReducer, useRef, useState, version)

I've encountered this issue after updating Vite, in that internal components library, from version 4.4.12 to 5.1.3. Vite uses esbuild for client builds minification by default, and in 5.0 they bumped its required esbuild version from ^0.18.0 to ^0.19.0. This is how the useId hook is called in the output code in that component library:

image

As a result, when I'm trying to build my main app (that uses the components library) using webpack, the build fails.

Expected behavior

Bundled maybeReactUseId variable (or any other mangled name), should probably use a bracket notation even when minifySyntax is enabled on esbuild@0.19.0 (or newer).

Context

I have two packages:

  • components library:
  1. react 17.0.2
  2. @mui/material 5.15.10
  3. vite 5.1.3
  4. esbuild 0.19.12
  • a main app that uses the above components library as a dependency:
  1. react 17.0.2
  2. webpack 5.90.3

When I'm trying to build the main app using webpack, I'm getting the error described above. It is possible to work around that by setting minifySyntax: false in esbuild settings but it'd be great that wouldn't turn out to be the only way.

Your environment

npx @mui/envinfo A browser doesn't matter in that case.
  System:
    OS: macOS 14.3.1
  Binaries:
    Node: 18.18.2 - ~/.nvm/versions/node/v18.18.2/bin/node
    npm: 9.8.1 - ~/.nvm/versions/node/v18.18.2/bin/npm
    pnpm: Not Found
  Browsers:
    Chrome: 121.0.6167.184
    Edge: Not Found
    Safari: 17.3.1
  npmPackages:
    @emotion/react: ^11.11.0 => 11.11.0 
    @emotion/styled: ^11.11.0 => 11.11.0 
    @mui/base:  5.0.0-beta.36 
    @mui/core-downloads-tracker:  5.15.10 
    @mui/material: ^5.15.10 => 5.15.10 
    @mui/private-theming:  5.15.9 
    @mui/styled-engine:  5.15.9 
    @mui/system:  5.15.9 
    @mui/types:  7.2.13 
    @mui/utils:  5.15.9 
    @types/react: ^17.0.0 => 17.0.59 
    react: ^17.0.0 => 17.0.2 
    react-dom: ^17.0.0 => 17.0.2 
    typescript: ^5.0.4 => 5.0.4 

Search keywords: esbuild useId utils webpack vite react

@patrykszwed patrykszwed added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 19, 2024
@zannager zannager added the package: system Specific to @mui/system label Feb 20, 2024
@misteroh10302
Copy link

misteroh10302 commented Feb 20, 2024

I'm getting the same issue but with useSyncExternalStore

const maybeReactUseSyncExternalStore: undefined | any = (React as any)['useSyncExternalStore' + ''];

@brijeshb42 brijeshb42 removed the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Feb 20, 2024
@brijeshb42
Copy link
Contributor

@patrykszwed Could you also provide a minimal repro for the repo where you finally use this built package ?

@brijeshb42 brijeshb42 added the status: waiting for author Issue with insufficient information label Feb 21, 2024
@patrykszwed
Copy link
Author

patrykszwed commented Feb 21, 2024

@brijeshb42 sure thing. I've updated the one linked in the description. It is enough to:

  1. cd final-app
  2. Run npm i
  3. Run npm run build

A warning about useId import should appear, and some additional ones about useInsertionEffect with the same message. Apparently useInsertionEffect causes a similar issue. Here are the logs:

Logs

➜ final-app git:(main) npm run build

final-app@1.0.0 build
webpack

asset bundle.js 421 KiB [emitted] [minimized] [big] (name: main) 1 related asset
orphan modules 417 KiB [orphan] 1 module
runtime modules 221 bytes 1 module
cacheable modules 684 KiB
modules by path ../ 134 KiB
modules by path ../node_modules/react/ 7.63 KiB 4 modules
modules by path ../node_modules/react-dom/ 119 KiB 2 modules
modules by path ../node_modules/scheduler/ 4.91 KiB 2 modules
../node_modules/object-assign/index.js 2.06 KiB [built] [code generated]
modules by path ./ 550 KiB
modules by path ./node_modules/react-dom/ 119 KiB 2 modules
modules by path ./node_modules/react/ 6.48 KiB 2 modules
modules by path ./node_modules/scheduler/ 4.91 KiB 2 modules
./src/index.js + 1 modules 417 KiB [built] [code generated]
./node_modules/object-assign/index.js 2.06 KiB [built] [code generated]

WARNING in ../lib/esm/index.js 1864:24-48
export 'useInsertionEffect' (imported as 'React') was not found in 'react' (possible exports: Children, Component, Fragment, Profiler, PureComponent, StrictMode, Suspense, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, cloneElement, createContext, createElement, createFactory, createRef, forwardRef, isValidElement, lazy, memo, useCallback, useContext, useDebugValue, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useReducer, useRef, useState, version)
@ ./src/index.js 2:0-56 4:42-54

WARNING in ../lib/esm/index.js 1864:51-75
export 'useInsertionEffect' (imported as 'React') was not found in 'react' (possible exports: Children, Component, Fragment, Profiler, PureComponent, StrictMode, Suspense, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, cloneElement, createContext, createElement, createFactory, createRef, forwardRef, isValidElement, lazy, memo, useCallback, useContext, useDebugValue, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useReducer, useRef, useState, version)
@ ./src/index.js 2:0-56 4:42-54

WARNING in ../lib/esm/index.js 2431:22-34
export 'useId' (imported as 'React6') was not found in 'react' (possible exports: Children, Component, Fragment, Profiler, PureComponent, StrictMode, Suspense, __SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED, cloneElement, createContext, createElement, createFactory, createRef, forwardRef, isValidElement, lazy, memo, useCallback, useContext, useDebugValue, useEffect, useImperativeHandle, useLayoutEffect, useMemo, useReducer, useRef, useState, version)
@ ./src/index.js 2:0-56 4:42-54

WARNING in asset size limit: The following asset(s) exceed the recommended size limit (244 KiB).
This can impact web performance.
Assets:
bundle.js (421 KiB)

WARNING in entrypoint size limit: The following entrypoint(s) combined asset size exceeds the recommended limit (244 KiB). This can impact web performance.
Entrypoints:
main (421 KiB)
bundle.js

WARNING in webpack performance recommendations:
You can limit the size of your bundles by using import() or require.ensure to lazy load some parts of your application.
For more info visit https://webpack.js.org/guides/code-splitting/

webpack 5.90.3 compiled with 6 warnings in 8831 ms

Here's a link to the repo itself - mui-repro-lib. Let me know in case you need anything else and thanks for looking into this.

@github-actions github-actions bot added status: waiting for maintainer These issues haven't been looked at yet by a maintainer and removed status: waiting for author Issue with insufficient information labels Feb 21, 2024
@brijeshb42
Copy link
Contributor

brijeshb42 commented Feb 21, 2024

Thanks. I will investigate.
Edit: @patrykszwed Can you update the demo app to be runnable in dev mode. Right now, it just renders Cannot GET / with a server error. Ideally, I should be able to see the Autocomplete component or some error in the browser console.

@patrykszwed
Copy link
Author

@brijeshb42 sure, I've added and setup a dev script so you should be able to run npm run dev now. It will run the app with errors unless Autocomplete import and usage is removed/commented out in the index.js file. These changes are merged into the repo and the sandbox.

@oliviertassinari oliviertassinari added package: base-ui Specific to @mui/base and removed package: system Specific to @mui/system labels Feb 25, 2024
@oliviertassinari oliviertassinari changed the title [utils] "useId" import error after updating esbuild [base-ui] "useId" import error after updating esbuild Feb 25, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2024

Reproduction

I can reproduce on https://esbuild.github.io/try/#dAAwLjE5LjYALS1taW5pZnkAcmV0dXJuICdzZWYnLnRvU3RyaW5nKCk7

Screen.Recording.2024-02-25.at.16.15.33.mov

This is since esbuild v0.19.6 (Nov 19, 2023).

Context

So If I understand correctly:

SCR-20240225-opxl
  • Next.js uses SWC as a minifier, but it looks super dumb so we are good
SCR-20240225-oqwp

Options

const use = 'use';
return `${use}Id`;

cc @atomiks too as https://github.com/floating-ui/floating-ui/blob/495954da10d2abe8a5e6a0f0247582f071d6f031/packages/react/src/hooks/useId.ts#L30 is a dependency of @mui/base

SCR-20240225-pafg

https://npm.anvaka.com/#/view/2d/%2540mui%252Fbase


SCR-20240225-ooqf

@brijeshb42 The @mui/utils package is mostly an npm dependency of Base UI, I fixed the labels (a bit MUI System too but it's rarer). I fixed this.

@oliviertassinari oliviertassinari added bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Feb 25, 2024
@atomiks
Copy link
Contributor

atomiks commented Feb 25, 2024

cc @atomiks too as https://github.com/floating-ui/floating-ui/blob/495954da10d2abe8a5e6a0f0247582f071d6f031/packages/react/src/hooks/useId.ts#L30 is a dependency of @mui/base

useId is not part of @floating-ui/react-dom, only @floating-ui/react, so the dependency won't affect @mui/base.

So simply passing an argument like 'useId'.toString(0) is what works now? There's a chance more bundler minifier updates will break it in the future, so maybe an alternative method is worth looking into.

Here's how Ariakit does it:

import * as React from "react";
// See https://github.com/webpack/webpack/issues/14814
const _React = { ...React };
const useReactId = _React.useId;

@brijeshb42
Copy link
Contributor

I've also verified that React['useId '.trim()] works but it'll be a cat and mouse game. We'll need to also get the similar issue fixed in emotion related to its usage of useInsertionEffect as that is also part of the error right now.

@brijeshb42
Copy link
Contributor

@patrykszwed Just curious, why not keep @mui/material as external when building your own package ?

@patrykszwed
Copy link
Author

@brijeshb42 our setup is pretty wild and we're using yarn in the app repo while using npm in the components package. When working locally, we're using links to link the local package version.

Juggling between different webpack and rollup configurations results, in most cases, in either:

  1. Invalid hook call. error (multiple react versions detected). We solve this by specifying an alias for react in our webpack config, that is: alias: { react: require.resolve('react') }
  2. Or when defining @mui/material as an external dependency - Module not found: Error: Can't resolve 'react/jsx-runtime' in '{MY_LOCAL_PATH}/node_modules/@mui/system/esm/cssVars'. This one can be solved by removing an alias for react, but once that's done we're going back to the first issue. We've been trying to fix that by using webpack fallback and/or alias options but without luck so far.

@patrykszwed
Copy link
Author

Hey @brijeshb42, I just wanted to ask if there's a plan to fix that issue in the nearest future? I took a look into this PR in the radix-ui repo. It looks like the solution proposed by its author (calling .trim() before the .toString() call) works so it could potentially be reused here as well.

esbuild v0.19.6:
https://esbuild.github.io/try/#dAAwLjE5LjYALS1taW5pZnkAcmV0dXJuICdzZWYnLnRyaW0oKS50b1N0cmluZygpOw
image

esbuild v0.20.2:
https://esbuild.github.io/try/#dAAwLjIwLjIALS1taW5pZnkAcmV0dXJuICdzZWYnLnRyaW0oKS50b1N0cmluZygpOw
image

@atomiks
Copy link
Contributor

atomiks commented Apr 6, 2024

I think the goal is to make it impossible to statically analyze. I saw a screenshot where React used Math.random() to trick bundlers, and it seems like the only future-proof method. This should hopefully work forever, regardless of new tricks bundlers perform:

React[`useId${Math.random()}`.slice(0, 5)]

The other technique that might be future proof is:

const _React = {...React};
const useReactId = _React.useId;

nwidger pushed a commit to nwidger/react-resizable-panels that referenced this issue Jul 31, 2024
Use technique from this comment: mui/material-ui#41190 (comment) to further obfuscate reference to `React.useId` which causes issue when using `react-resizable-panels` with React <18 and `esbuild` > 0.19.5.

Fixes bvaughn#381
bvaughn added a commit to bvaughn/react-resizable-panels that referenced this issue Aug 8, 2024
Use technique from this comment:
mui/material-ui#41190 (comment)
to further obfuscate reference to `React.useId` which causes issue when
using `react-resizable-panels` with React <18 and `esbuild` > 0.19.5.

Fixes #381

---------

Co-authored-by: Brian Vaughn <brian.david.vaughn@gmail.com>
@yash49
Copy link
Contributor

yash49 commented Aug 17, 2024

Hello team!
Thanks for all the great work.

We are also bundling MUI.
We haven't encountered the useId issue mentioned by @patrykszwed yet because we are using vite (esbuild version < v0.19.6).

However we are facing the issue for useMediaQuery because it uses concatenation
i.e. ['useSyncExternalStore' + ''] instead of ['useSyncExternalStore'.toString()]


I like the idea proposed by @atomiks

React[`useId${Math.random()}`.slice(0, 5)]

@oliviertassinari and @brijeshb42, I would love to contribute to this.

If you guys agree with atomiks's proposal then I can raise a PR.
Additionally, if you have any other solutions in mind, please let me know.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 17, 2024

I agree, we should fix this in Material UI too 👍.

On the options proposed here, I can barely find any differences between:

Option A

React[`useId${Math.random()}`.slice(0, 5)]

and, Option B

const _React = {...React};
const useReactId = _React.useId;

The runtime cost is almost the same, A: 0.005126953125 ms vs. B: 0.010986328125 ms. React could have tree-shakeable exports in the future facebook/react#11503, but both option looks equally good in that context. Option B might compress a bit better with brottli/gzip since it has more common constructs, e.g. {....

Going with what @atomiks used for Floating UI, since Base UI uses @mui/utils/useId, seems best, we will be consistent this way, so Option B.

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Aug 17, 2024
@yash49
Copy link
Contributor

yash49 commented Aug 18, 2024

@oliviertassinari I've raised the PR #43360
(I assume I can't modify the labels)

Please let me know if I've missed anything.
(This is my first open source contribution 😃)

Copy link

This issue has been closed. If you have a similar problem but not exactly the same, please open a new issue.
Now, if you have additional information related to this issue or things that could help future readers, feel free to leave a comment.

Note

We value your feedback @patrykszwed! How was your experience with our support team?
If you could spare a moment, we'd love to hear your thoughts in this brief Support Satisfaction survey. Your insights help us improve!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work external dependency Blocked by external dependency, we can’t do anything about it package: base-ui Specific to @mui/base ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
7 participants