Skip to content

Conversation

@michaldudak
Copy link
Member

As stores are accessible to developers with the handle prop on several components, I made it harder to accidentally update the state directly by making it read-only.

@michaldudak michaldudak requested a review from romgrk October 17, 2025 06:39
@michaldudak michaldudak added the package: utils Specific to the utils package. label Oct 17, 2025
@michaldudak michaldudak requested a review from atomiks as a code owner October 17, 2025 06:39
@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 17, 2025

vite-css-base-ui-example

pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/react@3005
pnpm add https://pkg.pr.new/mui/base-ui/@base-ui-components/utils@3005

commit: 00c490e

@netlify
Copy link

netlify bot commented Oct 17, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 00c490e
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/68f1e48e2c6c500009369dee
😎 Deploy Preview https://deploy-preview-3005--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@mui-bot
Copy link

mui-bot commented Oct 17, 2025

Bundle size report

Bundle Parsed size Gzip size
@base-ui-components/react 🔺+102B(+0.03%) 🔺+32B(+0.03%)

Details of bundle changes

Copy link
Contributor

@romgrk romgrk left a comment

Choose a reason for hiding this comment

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

I don't like this change, the state getter can lower performance a bit in extreme cases (like the Data Grid). In general I've tried to optimize the Store for performance first and simplicity second, so I aim to avoid defensive code patterns. I would prefer if we didn't merge this change, and we aimed to keep Store instances as an implementation detail - not accessible externally.

@atomiks
Copy link
Contributor

atomiks commented Oct 21, 2025

I would prefer if we didn't merge this change, and we aimed to keep Store instances as an implementation detail - not accessible externally.

I agree with this - the exposed store instance it should be changed to only export a subset of methods/properties, to avoid breaking changes

@michaldudak
Copy link
Member Author

We can't make it properly inaccessible from the outside, as store is a field in "handles" - data structures several components use to detach triggers.
I suppose it should be enough to mark the field as @internal in JSDOM and perhaps call it __store or similar instead to indicate it's not something to peek into.

@romgrk
Copy link
Contributor

romgrk commented Oct 23, 2025

Marking the field as internal/private is enough imo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: utils Specific to the utils package.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants