Skip to content

Conversation

Janpot
Copy link
Member

@Janpot Janpot commented Feb 5, 2025

  • data-mui-test was removed in [test] Remove data-mui-test from tests #23498. In the main babel config this means babel-plugin-react-remove-properties was a no-op. We can't really rely on it because the icons have a data-testid as documented public API.
  • data-testid pops are still in the docs. They're not removed from the icons as we're excluding babel on them. Here we can keep babel-plugin-react-remove-properties and reconfigure it to operate on ^data-test.
  • Took the opportunity to remove the babel-plugin-macros plugin which we don't use anymore. I didn't realize there was another babel config the docs folder. Looks like this was working because it's pulled in as a peer dependency by another package.
  • Removed a data-testid from markdown generated HTML.

Noticed this in #45208
Seeing about 4k occurrences of data-testid in the docs due to it being baked in the icons. We're not transpiling the icons package for performance reasons, and I imagine many with us. For some obscure reason we made this a public API, so we can't just remove them on a minor. Adding as an idea here.

We weren't seeing the warning in Next.js because of vercel/next.js#75682

@Janpot Janpot added docs Improvements or additions to the documentation. scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd). labels Feb 5, 2025
@mui-bot
Copy link

mui-bot commented Feb 5, 2025

Netlify deploy preview

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

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against f259e95

@Janpot Janpot changed the title clean up data-testid in the docs [docs] clean up data-testid in the docs Feb 5, 2025
@Janpot Janpot changed the title [docs] clean up data-testid in the docs [code-infra] Remove data-testid from our builds Feb 5, 2025
@Janpot Janpot removed the docs Improvements or additions to the documentation. label Feb 5, 2025
@Janpot Janpot changed the title [code-infra] Remove data-testid from our builds [code-infra] Reconfigure react-remove-properties for the docs Feb 5, 2025
@Janpot Janpot changed the title [code-infra] Reconfigure react-remove-properties for the docs [code-infra] Reconfigure react-remove-properties babel plugin Feb 5, 2025
@Janpot Janpot marked this pull request as ready for review February 5, 2025 13:57
@oliviertassinari
Copy link
Member

oliviertassinari commented Feb 5, 2025

History:

Looking at the rest of the codebase, I only see the pickers components that depend on react-remove-properties to work. Unclear if those should be public API or not.

@Janpot
Copy link
Member Author

Janpot commented Feb 5, 2025

so that developers can test their icon logic, RTL, etc.

Couldn't they just provide the data-testid themselves as a prop? Now we're adding this to every icon in the dom, in production in our docs and for every end-user application. It feels like we're also imposing data-testids that the user didn't explicitly set. It may clash with their expectations.

@zannager zannager requested a review from siriwatknp February 6, 2025 08:22
Copy link
Member

@siriwatknp siriwatknp left a comment

Choose a reason for hiding this comment

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

👍 Thanks for the cleanup

@Janpot Janpot merged commit d39d720 into mui:master Mar 19, 2025
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: code-infra Involves the code-infra product (https://www.notion.so/mui-org/5562c14178aa42af97bc1fa5114000cd).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants