forked from mui/mui-x
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Janpot
wants to merge
4
commits into
alexfauquette:add-test
Choose a base branch
from
Janpot:fix-with-tsx
base: add-test
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 3 commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| import { expect } from 'chai'; | ||
| import { FormatterParams } from '@mui/x-charts/models/seriesType/config'; | ||
| import lineFormatter from '@mui/x-charts/LineChart/formatter'; | ||
|
|
||
| const seriesOrder = ['id1']; | ||
| const seriesDataset: FormatterParams<'line'>['series'] = { | ||
| id1: { | ||
| // useless info | ||
| type: 'line', | ||
| id: 'id1', | ||
| color: 'red', | ||
| // usefull info | ||
| dataKey: 'k', | ||
| }, | ||
| }; | ||
| const seriesData: FormatterParams<'line'>['series'] = { | ||
| id1: { | ||
| // useless info | ||
| type: 'line', | ||
| id: 'id1', | ||
| color: 'red', | ||
| // usefull info | ||
| data: [1, 2, 3], | ||
| }, | ||
| }; | ||
|
|
||
| const dataSet = [{ k: 1 }, { k: 2 }, { k: 3 }]; | ||
|
|
||
| describe.only('LineChart - formatter', () => { | ||
| describe('data from dataset', () => { | ||
| it('shoudld get data from the dataset', () => { | ||
| const results = lineFormatter({ seriesOrder, series: seriesDataset }, dataSet); | ||
| console.log(results); | ||
| }); | ||
| }); | ||
| }); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_ENVin 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 🙂.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E.g. so we catch bugs if https://github.com/merceyz/babel-plugin-optimize-clsx introduce some.
I'm personally not attached to Babel but to the underlying concept: I want transpilation capabilities, one way or another.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄 .
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@oliviertassinari
@babel/registercan'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.
There was a problem hiding this comment.
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
tsupin zero-config runtime mui/material-ui#40426 (comment)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 wheresatisfieswould 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: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.
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)
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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