Skip to content

[core] refactor: remove manual displayName #17845

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

Merged
merged 3 commits into from
May 19, 2025

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented May 14, 2025

Follow-up of mui/base-ui#1920
Depends on mui/mui-public#329

@romgrk romgrk added core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature labels May 14, 2025
@romgrk romgrk requested a review from a team May 14, 2025 19:15
@mui-bot
Copy link

mui-bot commented May 14, 2025

Deploy preview: https://deploy-preview-17845--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against d60f678

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 14, 2025
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label May 16, 2025
@romgrk romgrk requested a review from a team May 16, 2025 19:57
@romgrk romgrk merged commit e511a53 into mui:master May 19, 2025
23 checks passed
@romgrk romgrk deleted the refactor-babel-display-name branch May 19, 2025 06:55
@JCQuintas
Copy link
Member

I expected this to affect lines like https://github.com/mui/mui-x/blob/master/packages/x-tree-view/src/internals/plugins/useTreeViewItems/useTreeViewItems.test.tsx#L32

But it seems our babel files are not run on vitest.

After checking, our babel setup is not compatible with our current test setup, so would require some work to make them work together.

@Janpot FYI

@Janpot
Copy link
Member

Janpot commented May 19, 2025

But it seems our babel files are not run on vitest.

Ah no, that was by design, that's why I initially proposed to do it this way.

@JCQuintas
Copy link
Member

But it seems our babel files are not run on vitest.

Ah no, that was by design, that's why I initially proposed to do it this way.

Ah true, in that case we can try running Babel just with specific plugins

@romgrk
Copy link
Contributor Author

romgrk commented May 19, 2025

Why were the tests designed to run without the babel plugins? Context: #17903 (comment)

@Janpot
Copy link
Member

Janpot commented May 20, 2025

Why were the tests designed to run without the babel plugins?

2025 tooling almost exclusively is being built on top of js parsers and bundlers that work at "native speed". Enabling babel plugins in this kind of tooling is usually possible, but rather as an extra compile step that is significantly slower than the default. If babel is just here to do downleveling of modern js/ts, is it still worth it having to teach all of the tooling to speak babel?

The tests weren't specifically designed to work without babel, one of the ideas of moving to vitest was to reduce the reliance on babel so that eventually we can work towards removing this babel.config.js altogether, in favor for a post-production-build step should there be any mui-specific needs left. Leaving our source code maximally portable between engines and compilers as to make it easy to introduce and tweak tooling without every time imposing a slowdown.

And as we've noticed building the css support in X, babel as a build tool itself falls short for its purpose here. It's a 1-to-1 file transformation, but to do properly what you want it to do you need bundling capabilities. Where exists the notion of a module graph, and where is lots of control over the whole build lifecycle. Keeping the reliance on babel solely for downleveling purposes keeps the migration path easy to the next version of of our build pipeline. After core is on vitest, we likely want to explore moving to a build tool like vite, tanstack/config, or tsup to build our libraries. For better performance, and to be able to tap in to the rollup ecosystem for things like the css support.

So in the context of that mindset, the display name plugin may be a slight step backwards as it forces us to add a babel transform to the tests again potentially slowing it down.

We all make different trade-offs though, the above has been discussed a bit before in code-infra, but not extensively. I'd love to hear more perspectives.

About the displayName plugin, should we decide it's not substantial enough DX improvement to trade-in performance of our tooling, I do have an alternative: which is to put it back in the sources, but write an eslint plugin that can enforce it. It could be great DX if there's an autofix in the plugin as that would free us from having to write those manually. It also has the added benefit of leaving an easy way to override should we have specific cases where we want to do something different.

@romgrk
Copy link
Contributor Author

romgrk commented May 21, 2025

2025 tooling almost exclusively is being built on top of js parsers and bundlers that work at "native speed". Enabling babel plugins in this kind of tooling is usually possible, but rather as an extra compile step that is significantly slower than the default. If babel is just here to do downleveling of modern js/ts, is it still worth it having to teach all of the tooling to speak babel?

I'd personally love to be able to write these kind of transforms in a native language (Rust ideally), but I don't think that infrastructure has a good enough support for custom code transformations/plugins yet, and even if it had I'm not sure if we have enough native-fluent devs to maintain that code in MUI.

The problem with native tools is that they're still just transpiling standard JS/TS to standard JS, but they aren't made to optimize specific JS libraries, and I don't think babel is just for downleveling. For example, the react-compiler is used via a babel plugin, and it can do amazing optimizations that go beyond the scope of downleveling.

About the displayName plugin, should we decide it's not substantial enough DX improvement to trade-in performance of our tooling, I do have an alternative: which is to put it back in the sources, but write an eslint plugin that can enforce it. It could be great DX if there's an autofix in the plugin as that would free us from having to write those manually.

Isn't eslint just like babel, wrt to performance? Running eslint is in my experience very slow.

Using eslint over babel also goes against my DX philosophy. I prioritize reading-DX over writing-DX. The eslint plugin might make writing those less annoying, but all those lines still go in the source code and need to be read or skipped over everytime we work on a file. As an organization I feel like we don't care enough about reading-DX, and often when I open some files I have this wall-of-text effect where there's a lot of characters appearing on screen, most of them being irrelevant to the core business logic that I need to understand/update. For example, here is a block of code before/after I refactored it (part of it was the .displayName):

Before After
Screenshot From 2025-05-21 16-37-38 Screenshot From 2025-05-21 16-37-53

So that's why I still prefer using babel even though it slows down parts of the stack. I'll be happy to switch to whatever native alternative to it appears though.

@romgrk
Copy link
Contributor Author

romgrk commented May 21, 2025

Another small note, some of the native tools aren't mature enough yet, see these issues for SWC vs Babel. Feels like the early days of GCC vs Clang. Clang wins eventually, but it needs enough time to catch up.

@Janpot
Copy link
Member

Janpot commented May 23, 2025

The problem with native tools is that they're still just transpiling standard JS/TS to standard JS, but they aren't made to optimize specific JS libraries, and I don't think babel is just for downleveling. For example, the react-compiler is used via a babel plugin, and it can do amazing optimizations that go beyond the scope of downleveling.

Agreed, downleveling and optimization. The point is, anything that bundlers and runtimes are incentivezed to adopt natively. What doesn't fall under that umbrella is anything that alters the semantics of the language. e.g. we removed babel macros because it just turns your code in something that only babel can run. Portability is important here. And that's mostly important because I want the code to be accessible to newcomers. The only thing you should need to consult to understand the semantics of the code is the ECMA standard, the typescript docs.

Isn't eslint just like babel, wrt to performance? Running eslint is in my experience very slow.

Sure, but it's only one tool that is slower, instead of all tools. And if we ever want to switch to another tool, it's only one other tool to port plugins to, instead of all. We're also already running eslint, we already pay the cost. An extra plugin doesn't add much to this cost. Most of us run eslint on save, so it also just has to run on a single file at once most of the time.

I prioritize reading-DX over writing-DX.

Fully agreed, that's why I think we should be very strict about whether plugins can change semantics or not. So that readers don't need to know more than standard javascript (tyescript) when they read our code. Granted, the display name plugin rides the grey zone very well here. On one hand one could argue it changes semantics because it breaks the tests, on the other hand one could argue all it does is adding extra debugging infromation.

here is a block of code before/after I refactored it

I follow you point, but that code is mostly more readable for other reasons than the removal of displayName. You seem to have mainly improved the abstractions. Bringing back displayName wouldn't bring far less clutter than leaving displayName out and bringing back the other changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes enhancement This is not a bug, nor a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants