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

Use the import * as React from 'react' pattern #2242

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Conversation

RobinMalfait
Copy link
Member

We use named imports, but we have to import React itself as well for JSX because it compiles to React.createElement. We could get rid of our own JSX and use it directly, or we can use this import * as React from 'react' syntax.

This fixes an issue for people using allowSyntheticDefaultImports: false in TypeScript.

Fixes: #2117

We use named imports, but we have to import `React` itself as well for
JSX because it compiles to `React.createElement`. We could get rid of
our own JSX and use it directly, or we can use this `import * as React
from 'react'` syntax.

This fixes an issue for people using `allowSyntheticDefaultImports: false` in TypeScript.

Fixes: #2117
@vercel
Copy link

vercel bot commented Feb 2, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
headlessui-react ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 2, 2023 at 1:53PM (UTC)
headlessui-vue ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 2, 2023 at 1:53PM (UTC)

@RobinMalfait
Copy link
Member Author

The main downside I see is that because of * as React, it could be that the bundle size is bigger if tree-shaking isn't working properly. On the other hand we require the React variable to be in scope which has React.useEffect and all those other hooks already attached.

Afaik, those functions are relatively small, and the actual implementation lives in react-dom.

What are your thoughts on this, @thecrypticace?

@thecrypticace
Copy link
Contributor

i don't think the bundle size will change all that much. React doesn't really have a proper, tree-shakable ESM build, no? So the only change in bundle size will be due to codegen differences rather than tree-shaking. Even still import * as React would still allow static analysis in the same manner that import { … } would. With the synthetic default import we were using before it was pretty much guaranteed that tree-shaking was impossible before so I can only see this as likely being a net win if there even is a significant change at all.

@RobinMalfait RobinMalfait merged commit 0276231 into main Feb 2, 2023
@RobinMalfait RobinMalfait deleted the fix/issue-2117 branch February 2, 2023 14:34
@IgorKhomenko
Copy link

This change broke our app build with the following errors:

�[0m�[91m./node_modules/@headlessui/react/dist/hooks/use-id.js
Attempted import error: 'useId' is not exported from 'react' (imported as 't').
./node_modules/@headlessui/react/dist/utils/start-transition.js
Attempted import error: 'startTransition' is not exported from 'react' (imported as 'r').

So we were forced to pin @headlessui/react to 1.7.8 for now

@jeafreezy
Copy link

Same experience here as @IgorKhomenko . Pinning to "@headlessui/react": "1.7.7" solved it.

@thecrypticace
Copy link
Contributor

@IgorKhomenko @jeafreezy What build tools are you using?

@IgorKhomenko
Copy link

@thecrypticace we use Next.js 12.3.4

@jeafreezy
Copy link

@thecrypticace we use Next JS 12.2.0

RobinMalfait added a commit that referenced this pull request Feb 6, 2023
@RobinMalfait
Copy link
Member Author

I reverted this change, update to the latest version and it should be fixed!

/cc @jeafreezy @IgorKhomenko @thecrypticace

@jeafreezy
Copy link

jeafreezy commented Feb 6, 2023

Works fine now on my end. Great job, Thanks!

@xsjcTony
Copy link

xsjcTony commented Feb 8, 2023

@RobinMalfait Does that mean I have to turn on allowSyntheticDefaultImports without any other choices?

@RobinMalfait
Copy link
Member Author

@xsjcTony until we have a better solution, yes I think so. I think it's also the default in (most) cases.

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

Successfully merging this pull request may close these issues.

TypeError of Dialog and Dialog.Title from the official example
5 participants