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] Fix downstream bundlers remove React 17 useId compatibility #37183

Merged
merged 1 commit into from
May 16, 2023

Conversation

nickiaconis
Copy link
Contributor

Summary

Fixes #37182. Based on this commit from floating-ui. I've confirmed that this will resolve the issue by manually tweaking the useId code in a local install of @mui/utils and running it through the same bundle process that's stripping the string concatenation.

Bundle output before tweak to confirm fix, using string concatenation in utils source

Note that +"" has been stripped from React3["useId"+""] on line 15. This is what's causing the downstream build issue.

// ../../common/temp/node_modules/.pnpm/@mui+utils@5.11.13_react@17.0.2/node_modules/@mui/utils/esm/useId.js
import * as React3 from "react";
var globalId = 0;
function useGlobalId(idOverride) {
  const [defaultId, setDefaultId] = React3.useState(idOverride);
  const id = idOverride || defaultId;
  React3.useEffect(() => {
    if (defaultId == null) {
      globalId += 1;
      setDefaultId(`mui-${globalId}`);
    }
  }, [defaultId]);
  return id;
}
var maybeReactUseId = React3["useId"];
function useId2(idOverride) {
  if (maybeReactUseId !== void 0) {
    const reactId = maybeReactUseId();
    return idOverride != null ? idOverride : reactId;
  }
  return useGlobalId(idOverride);
}

Bundle output after tweak to confirm fix, using .toString() in utils source

Note that the call to toString has been preserved on line 15.

// ../../common/temp/node_modules/.pnpm/@mui+utils@5.11.13_react@17.0.2/node_modules/@mui/utils/esm/useId.js
import * as React3 from "react";
var globalId = 0;
function useGlobalId(idOverride) {
  const [defaultId, setDefaultId] = React3.useState(idOverride);
  const id = idOverride || defaultId;
  React3.useEffect(() => {
    if (defaultId == null) {
      globalId += 1;
      setDefaultId(`mui-${globalId}`);
    }
  }, [defaultId]);
  return id;
}
var maybeReactUseId = React3["useId".toString()];
function useId(idOverride) {
  if (maybeReactUseId !== void 0) {
    const reactId = maybeReactUseId();
    return idOverride != null ? idOverride : reactId;
  }
  return useGlobalId(idOverride);
}

@mui-bot
Copy link

mui-bot commented May 5, 2023

Netlify deploy preview

https://deploy-preview-37183--material-ui.netlify.app/

Bundle size report

Details of bundle changes

Generated by 🚫 dangerJS against fb06a45

@zannager zannager added the package: system Specific to @mui/system label May 8, 2023
@zannager zannager requested a review from mnajdova May 8, 2023 07:47
@mnajdova
Copy link
Member

mnajdova commented May 8, 2023

Hey, thanks for looking into this. Can you please share simple reproduction app so that we can validate the fix?

@nickiaconis
Copy link
Contributor Author

nickiaconis commented May 8, 2023

Hi @mnajdova, would it be sufficient to only create a simple reproduction library, since we can look at the bundle output to see if it retains or loses v17 compatibility?

@nickiaconis
Copy link
Contributor Author

If so, here it is: simple reproduction library

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@nickiaconis
Copy link
Contributor Author

Thanks for the review @mnajdova! What's the next step in the process here?

@nickiaconis
Copy link
Contributor Author

Wanted to check back in on this since it's a blocker for us. What needs to happen to get this merged?

@siriwatknp
Copy link
Member

Wanted to check back in on this since it's a blocker for us. What needs to happen to get this merged?

It should be released today!

@siriwatknp siriwatknp merged commit 3a8ad7e into mui:master May 16, 2023
5 of 6 checks passed
binh1298 pushed a commit to binh1298/material-ui that referenced this pull request May 17, 2023
@oliviertassinari oliviertassinari changed the title [Utils] fix downstream bundlers remove React 17 useId compatibility [utils] Fix downstream bundlers remove React 17 useId compatibility Feb 25, 2024
@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] Fix downstream bundlers remove React 17 useId compatibility [base-ui] Fix downstream bundlers remove React 17 useId compatibility Feb 25, 2024
@oliviertassinari oliviertassinari added the bug 🐛 Something doesn't work label Feb 25, 2024
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 25, 2024

This PR is strange, we update useId but not useSyncExternalStore? We have two different solutions for the same problem:

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

I have left notes in #41190 to fix all instances.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Something doesn't work package: base-ui Specific to @mui/base
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[base-ui] downstream bundlers strip React 17 useId fix
6 participants