Skip to content

[internal] Refactor data passing - #1249

Merged
atomiks merged 9 commits into
mui:masterfrom
atomiks:refactor/whole-objects
Jan 8, 2025
Merged

[internal] Refactor data passing#1249
atomiks merged 9 commits into
mui:masterfrom
atomiks:refactor/whole-objects

Conversation

@atomiks

@atomiks atomiks commented Dec 30, 2024

Copy link
Copy Markdown
Contributor

Part of #1246. Focuses just on anchored popups in this PR

@mui-bot

mui-bot commented Dec 30, 2024

Copy link
Copy Markdown

Netlify deploy preview

https://deploy-preview-1249--base-ui.netlify.app/

Generated by 🚫 dangerJS against 7ce327b

@atomiks atomiks added the core label Dec 30, 2024
@atomiks
atomiks force-pushed the refactor/whole-objects branch from 458923a to ee76ae6 Compare December 30, 2024 09:50
const {
const tooltipRoot = useTooltipRoot({
...props,
defaultOpen,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the point of passing onOpenChange, open, and other props explicitly if they are a part of ...props?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, you're right. Forgot that the linting allows unused variables (as they need to be destructured to be documented)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well not anymore with our latest linting config:

/tmp/base-ui/packages/react/src/popover/root/PopoverRoot.tsx
16:11 error 'defaultOpen' is assigned a value but never used @typescript-eslint/no-unused-vars
16:24 error 'open' is assigned a value but never used @typescript-eslint/no-unused-vars
16:30 error 'onOpenChange' is assigned a value but never used @typescript-eslint/no-unused-vars
...
✖ 12 problems (12 errors, 0 warnings)

@michaldudak

Copy link
Copy Markdown
Member

Some cases could be simplified even more (less destructuring) if we passed all the props to hooks as they are, without providing default values on a component level. I'm also thinking about reconsidering the context access pattern we use and having the hooks instead of components read context values. This way, we might not need to create new objects just to pass data to hooks as a single parameter.

@atomiks

atomiks commented Jan 3, 2025

Copy link
Copy Markdown
Contributor Author

Some cases could be simplified even more (less destructuring) if we passed all the props to hooks as they are, without providing default values on a component level.

Are you referring to useAnchorPositioning or other ones too? A lot of the defaults are the same for that one, although a couple are different for each component (like side). Also, doesn't the documentation require the destructuring?

@michaldudak

Copy link
Copy Markdown
Member

Are you referring to useAnchorPositioning or other ones too?

I mean all our hooks in general (especially the simpler ones). In cases like https://github.com/mui/base-ui/blob/master/packages/react/src/popover/title/PopoverTitle.tsx#L25, we might simplify passing parameters by passing the whole props object to usePopoverTitle and moving usePopoverRootContext to the hook. I see that the Select hooks already read context. The perf gains won't be huge, so it's likely not worth going through the whole codebase, but it could be something to consider in future components.

Also, doesn't the documentation require the destructuring

Yup, we'd have to change the docgen if we wanted to do this.

@atomiks
atomiks force-pushed the refactor/whole-objects branch from 2b70f5d to ee76ae6 Compare January 3, 2025 08:41
@atomiks
atomiks marked this pull request as ready for review January 3, 2025 08:42
@atomiks
atomiks requested a review from colmtuite as a code owner January 3, 2025 08:42
@github-actions github-actions Bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 7, 2025
Signed-off-by: atomiks <cc.glows@gmail.com>
@github-actions github-actions Bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Jan 7, 2025
@atomiks
atomiks merged commit bcbceda into mui:master Jan 8, 2025
@atomiks
atomiks deleted the refactor/whole-objects branch January 8, 2025 07:29
@oliviertassinari oliviertassinari added internal Behind-the-scenes enhancement. Formerly called “core”. and removed core labels Aug 2, 2025
@oliviertassinari oliviertassinari changed the title [core] Refactor data passing [internal] Refactor data passing Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal Behind-the-scenes enhancement. Formerly called “core”.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants