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

Remove dot-notation support #346

Merged
merged 2 commits into from
Mar 7, 2024
Merged

Conversation

benoitgrelard
Copy link
Contributor

No description provided.

@benoitgrelard benoitgrelard requested a review from vladmoroz March 7, 2024 10:58
Copy link

vercel bot commented Mar 7, 2024

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

Name Status Preview Comments Updated (UTC)
themes-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 7, 2024 11:03am

@benoitgrelard benoitgrelard merged commit a56bafb into main Mar 7, 2024
2 checks passed
@benoitgrelard benoitgrelard deleted the remove-dot-notation-support branch March 7, 2024 11:21
@Nedomas
Copy link

Nedomas commented Mar 7, 2024

Hi! Do you guys have more info on why this pattern should be avoided?

Just found one post by @gaearon vercel/next.js#51593 (comment) that maybe import * as Popover from '@radix-ui/themes' would be ok.

Thanks for all the great work by the way!

@vladmoroz
Copy link
Contributor

Hi! Do you guys have more info on why this pattern should be avoided?

Just found one post by @gaearon vercel/next.js#51593 (comment) that maybe import * as Popover from '@radix-ui/themes' would be ok.

Thanks for all the great work by the way!

Hey, we love dot notation, but unfortunately, it doesn't work with RSC and we see no acknowledgement from framework maintainers like Next.js whether there are plans to fix it. We've had a few "bug reports" when people imported components with dot notation and couldn’t understand why it would blow up their app.

Since right now the package is polluted with both export styles, it makes the experience in the IDE worse overall than having a single way to import the components.

I wish it was the other way around as dot notation is an objectively better experience with multi-part components, but there is no point in having it if it doesn't work in modern frameworks and brings confused people to file bug reports here.

import * as Popover from '@radix-ui/themes'

This can’t work for importing a specific component. * imports all exports from the package. It works with primitives though because each primitive is its own package, e.g:

import * as Popover from '@radix-ui/react-popover'

@hoop71
Copy link

hoop71 commented Mar 7, 2024

RIP dot notation. You were superior and you will be missed. 😞

@gaearon
Copy link

gaearon commented Mar 8, 2024

unfortunately, it doesn't work with RSC and we see no acknowledgement from framework maintainers like Next.js whether there are plans to fix it.

Please note this isn't about any specific frameworks per se — there are no plans to fix it from the React side itself because patched-on properties like Foo.Bar are not a module system concept, and we only support module system concepts (module imports/exports) being addressed via the network boundary. However, import * is a module system concept so as noted above it works in RSC (and so it works for individual packages as @vladmoroz mentioned).

@penx
Copy link
Contributor

penx commented Mar 8, 2024

patched-on properties like Foo.Bar are not a module system concept, and we only support module system concepts (module imports/exports)

but is this a patched on property? I addressed this recently so that the Radix Themes exports use the module system:

#156

@penx
Copy link
Contributor

penx commented Mar 8, 2024

We've had a few "bug reports" when people imported components with dot notation and couldn’t understand why it would blow up their app

Were these bugs when using a version before #156? From the sounds of @gaearon 's post above and linked comment, #156 would resolve this

@penx
Copy link
Contributor

penx commented Mar 8, 2024

it doesn't work with RSC... We've had a few "bug reports"

Is there a linked issue or recreation, either here or upstream? I would be interested to try it out with a version that had #156 in it, e.g. 3.0.0-rc.14-patch.1

@gaearon
Copy link

gaearon commented Mar 8, 2024

Ah interesting, I don't know whether export * as would work, but it's worth checking.

@vladmoroz
Copy link
Contributor

So... we double checked and export * as that @penx has recently introduced does work with RSC 🥹

Glad that this discussion happened—we were operating on the assumption that this technique of re-exporting modules internally within the package wouldn't have changed anything, but we were wrong. In the hindsight it makes sense—exporting objects is not the same as group export, as object assignment happens at runtime.

Guess it means we will be doing the opposite—we'll remove the individual part exports instead 🙂

penx added a commit to penx/themes that referenced this pull request Mar 11, 2024
benoitgrelard pushed a commit that referenced this pull request Mar 11, 2024
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.

6 participants