Skip to content
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

One way to make the tests run #11

Open
wants to merge 4 commits into
base: add-test
Choose a base branch
from

Conversation

Janpot
Copy link

@Janpot Janpot commented Jan 4, 2024

Proposing a fix for mui#11551

Replacing the build step with tsx seems to magically make it run. I didn't run it on the rest of the test suite, so you may want to investigate whether this setup fully works first

@Janpot
Copy link
Author

Janpot commented Jan 4, 2024

cc @michaldudak You might want to take a look at this whether it could help with core. It also transfers ownership of aliasing from babel to tsconfig for mocha tests.

.mocharc.js Outdated
require: [require.resolve('./test/utils/setupBabel'), require.resolve('./test/utils/setupJSDOM')],
'node-option': ['loader=tsx/cjs'],
Copy link

@oliviertassinari oliviertassinari Jan 4, 2024

Choose a reason for hiding this comment

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

Would it continue to run babel.config.js? IMHO we need the test to run the same Babel plugins that run when publishing to npm.

Copy link
Author

@Janpot Janpot Jan 4, 2024

Choose a reason for hiding this comment

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

Would it continue to run babel.config.js? IMHO we need the test to run the same Babel plugins that run when publishing to npm.

It wouldn't, personally I don't really see how much value that adds. We run these mocha tests in a completely different environment than our users anyway. i.e. native Node.js, while most of our users run it in browsers through a bundler, that compiles each file down to their target env anyway.

It already doesn't run the same plugins by the way, looking at the amount of branching that is happening. + it runs tests for only one babel configuration while we publish multiple different builds. And we run with different NODE_ENV in test, compared to production, which enables yet another set of plugins. If anything, for the long term I'd advocate for progressively moving our builds away from babel. Too slow and too much complexity in 2024 🙂.

Copy link

@oliviertassinari oliviertassinari Jan 4, 2024

Choose a reason for hiding this comment

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

I don't really see how much value it adds.

E.g. so we catch bugs if https://github.com/merceyz/babel-plugin-optimize-clsx introduce some.

advocate for progressively moving our builds away from babel.

I'm personally not attached to Babel but to the underlying concept: I want transpilation capabilities, one way or another.

Copy link
Author

@Janpot Janpot Jan 4, 2024

Choose a reason for hiding this comment

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

I want transpilation capabilities, one way or another.

Ok, I'll look for another way to make this work with babel then, if we're not long term planning on removing it then let's try to keep everything on the same toolchain. There is no alternative atm that makes it easy to build/pull custom plugins like babel can.

I'm of the opposite opinion though, given the amount of whack-a-mole we're playing with ESM issues, and the dwindling support of tools for babel, and the slowness, I'd rather remove the complexity of the few babel plugins we use altogether. IMO a simple convention would make more sense than installing babel just for things like https://github.com/merceyz/babel-plugin-optimize-clsx, it's like asking for a banana and getting the gorilla holding it, together with the whole jungle 😄 .

Copy link
Author

Choose a reason for hiding this comment

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

@oliviertassinari @babel/register can't handle pure ESM modules like d3-* packages it seems. But we can just run both babel and tsx I guess, the latter seems to be able to handle it just fine, not sure what babel is talking about.

This installs both the babel and tsx hooks, which is somewhat redundant, but it's the best I can offer if we want to run babel plugins.

Copy link
Author

Choose a reason for hiding this comment

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

Also noticed that babel is being dropped in favour for tsup in zero-config runtime mui/material-ui#40426 (comment)

Copy link

@oliviertassinari oliviertassinari Jan 9, 2024

Choose a reason for hiding this comment

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

it's like asking for a banana and getting the gorilla holding it, together with the whole jungle

Haha.

I think that the problem is that our users don't care if our dev tools are slow as long as their components are performant in production. We rely on a couple of Babel plugins to make this work. https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types probably has the most impact in terms of improving DX without harming bundle size. Before TypeScript was a thing, that plugin used to be popular (as a % of react-dom):

https://npm-stat.com/charts.html?package=react-dom&package=babel-plugin-transform-react-remove-prop-types&from=2020-01-08&to=2024-01-08

But in any cases, the zero runtime is using Babel to do the transpilation, so sounds like we are digging our hole deeper 😄.

It seems that only Next.js is pushing the SWC narrative: https://npm-stat.com/charts.html?package=%40swc%2Fcore&package=%40babel%2Fcore&package=next&from=2023-01-08&to=2024-01-08.

And wow, it looks like Vite is pushing the Rollup narrative, I wouldn't have expected Rollup to take over Webpack https://npm-stat.com/charts.html?package=webpack&package=rollup&package=vite&from=2023-01-08&to=2024-01-08. We will see if Turbopack changes this.

Copy link
Author

@Janpot Janpot Jan 9, 2024

Choose a reason for hiding this comment

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

I think that the problem is that our users don't care if our dev tools are slow as long as their components are performant in production. We rely on a couple of Babel plugins to make this work.
https://github.com/oliviertassinari/babel-plugin-transform-react-remove-prop-types probably has the most impact in terms of improving DX without harming bundle size. Before TypeScript was a thing, that plugin used to be popular (as a % of react-dom):

It is not only about slowness. It is also about being able to use certain features. e.g. Next.js+babel doesn't support typescript satisfies. I've noticed instances in the X codebase where satisfies would lead to better typings. And end-users do care about that. e.g. As a way to better narrow down column definitions for certain column types. The problem is not that we're using babel, it's that we've locked ourselves in it. With very little benefit, things that can be done with techniques that exist in every compiler. e.g. variable replacement and dead code elimination:

const Baz = (props) => <div {...props} />;

if (process.env.NODE_ENV === 'development') {
  Baz.propTypes = {
    className: PropTypes.string,
  };
}

I'm totally fine to use babel as a compiler where appropriate, I just wish we were able to use other tools as well. Now the slowness of babel dictates the speed of every tool we use.

But in any cases, the zero runtime is using Babel to do the transpilation, so sounds like we are digging our hole deeper 😄.

It's built on top of babel, but it's compiled using esbuild. They used the most appropriate tool for the job at this point in time. babel because easily extendible with javascript, esbuild because it's fast. (I'm not fully sure we should be building it in the first place 🙂. I sense some babel plugin fatigue in the community, I for one wouldn't use this if it mandated compiling all my sources with babel)

It seems that only Next.js is pushing the SWC narrative: https://npm-stat.com/charts.html?package=%40swc%2Fcore&package=%40babel%2Fcore&package=next&from=2023-01-08&to=2024-01-08.

Yep, vite is pushing the esbuild narrative, look where the growth is https://npm-stat.com/charts.html?package=%40swc%2Fcore&package=%40babel%2Fcore&package=esbuild&from=2023-01-08&to=2024-01-08
The point is not to move to SWC across the repo, it's about being able to use SWC in Next.js and babel or tsup to compile our bundles. It's about keeping the source code as close to the standards as possible to have it as portable as possible.

And wow, it looks like Vite is pushing the Rollup narrative, I wouldn't have expected Rollup to take over Webpack https://npm-stat.com/charts.html?package=webpack&package=rollup&package=vite&from=2023-01-08&to=2024-01-08. We will see if Turbopack changes this.

vite is porting rollup to rust as we speak (https://vitejs.dev/blog/announcing-vite5) and planning to move their whole toolchain to Rust. Vite + React is primed to win 2024 https://npm-stat.com/charts.html?package=vite&package=%40vitejs%2Fplugin-react&package=next&from=2022-01-07&to=2024-01-07. We should prioritize vitejs/vite#12423 if we want to stay relevant with those users.

Choose a reason for hiding this comment

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

Ok, this all makes sense to me.

Copy link
Author

@Janpot Janpot Jan 10, 2024

Choose a reason for hiding this comment

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

Whoops, just found out that rollup is pushing the SWC narrative after all, it just doesn't reflect in the npm downloads as they seem to compile from source: rollup/rollup#5073

I'm not 100% sure but I suspect that means that vite plans to switch from esbuild to SWC some day. It wouldn't make sense to use both swc and esbuild.

And I'm pretty sure that turbopack is also built on top of SWC, but also from source.

This makes SWC much bigger than the npm stats reflect

@Janpot Janpot closed this Jan 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants