Skip to content

[internal] Fix proptypes generation when multiple components per file#44058

Merged
Janpot merged 7 commits intomui:masterfrom
Janpot:fix-prop-types
Oct 10, 2024
Merged

[internal] Fix proptypes generation when multiple components per file#44058
Janpot merged 7 commits intomui:masterfrom
Janpot:fix-prop-types

Conversation

@Janpot
Copy link
Copy Markdown
Member

@Janpot Janpot commented Oct 9, 2024 •
edited
Loading

Reviewing proptypes generation code for mui/mui-public#209, I noticed this bug. original proptypes were replaced without checking for multiple instances in the file. I'm replacing the single originalPropTypesPath with a map that maps each component to its own originalPropTypesPath.

This generated some new proptypes for two memoized components, which doesn't support a .propTypes property, this lead to build errors. I assign the inner components to intermediate variables to solve those.

It looks like there's a similar thing going on with previousPropTypesSource as well, applied the same fix, which didn't result in any changes.

@Janpot Janpot added the internal Behind-the-scenes enhancement. Formerly called “core”. label Oct 9, 2024
@mui-bot
Copy link
Copy Markdown

mui-bot commented Oct 9, 2024 •
edited
Loading

Netlify deploy preview

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

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against b3da151

@Janpot Janpot marked this pull request as ready for review October 10, 2024 09:41
@michaldudak
Copy link
Copy Markdown
Member

michaldudak commented Oct 10, 2024

The newly added proptypes don't add any value as they annotate internal components (and they duplicate what's already there in public components). We should add @ignore - internal component. JSDoc to prevent generating unnecessary code

@Janpot
Copy link
Copy Markdown
Member Author

Janpot commented Oct 10, 2024

We should add @ignore - internal component. JSDoc to prevent generating unnecessary code

Doesn't this flag work on a per-file basis? These files also contain a public component as far as I understand.

@michaldudak
Copy link
Copy Markdown
Member

michaldudak commented Oct 10, 2024

Right, in this case, it sucks :/
We should address it separately at some point.

@Janpot
Copy link
Copy Markdown
Member Author

Janpot commented Oct 10, 2024 •
edited
Loading

Didn't want to waste too much time on it and assumed it was better to rather have those proptypes there than the bug, even though it's at least 4 years old.

@Janpot Janpot merged commit f776273 into mui:master Oct 10, 2024
@LukasTy
Copy link
Copy Markdown
Member

LukasTy commented Oct 16, 2024 •
edited
Loading

After this change, we are facing problems on mui/mui-x#14937.
It fixes a few cases, where proptypes are expected (multiple exported components), but has a "regression" of forcing proptypes on components, which are not exported. 🙈
Adding the following comment to such components doesn't stop proptypes from being generated... 🤷

/**
 * @ignore - internal component.
 */

Any ideas on how to proceed with the mentioned PR? 🤔
cc @Janpot

@Janpot
Copy link
Copy Markdown
Member Author

Janpot commented Oct 16, 2024 •
edited
Loading

🤔 As far as I understand @ignore - internal component. has always worked at the file level. I believe we'll either have to:

  • keep the proptypes. Are they a problem?
  • make the ignore comment apply on the component level. (Not immediately obvious how)
  • move components we don't want proptypes on to their own file with the ignore comment

I remember trying this PR out on X, didn't see changes, I must have messed up.

@Janpot Janpot deleted the fix-prop-types branch October 16, 2024 13:49
@joshkel joshkel mentioned this pull request Oct 16, 2024
Closed
@LukasTy
Copy link
Copy Markdown
Member

LukasTy commented Oct 17, 2024

keep the proptypes. Are they a problem?

Ok, let's go with this approach. 👌
There was a slight problem, one of the generated proptypes was for a styled component, which fails tsc. 🤔
I've made some slight changes to avoid generation in that case. 👌

@Hoang-Nguyen-Huy Hoang-Nguyen-Huy mentioned this pull request Nov 9, 2024
Merged
@oliviertassinari oliviertassinari changed the title [core] Fix proptypes generation when multiple components per file [internal] Fix proptypes generation when multiple components per file Feb 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@michaldudak michaldudak michaldudak approved these changes

@siriwatknp siriwatknp Awaiting requested review from siriwatknp

Assignees

No one assigned

Labels

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Janpot @mui-bot @michaldudak @LukasTy