mui-x
mui-x copied to clipboard
[charts] Use vendor to have CJS working out of the box
Related issues
Would fix #11568 -: with the following modification the error is gone https://github.com/alexfauquette/mui-x-charts-jest-issue-minimal-reproduction/pull/1
For #11016 we still have some import issue: https://stackblitz.com/edit/stackblitz-starters-okvxu3?file=package.json,index.mjs I had to do the following modification to have the example work (plus adding share dependencies)
- import { BarChart } from '@mui/x-charts/BarChart';
+ import { BarChart } from '@mui/x-charts/BarChart/index.js';
Fix #9826 https://codesandbox.io/p/devbox/silly-turing-2xjgpx?file=%2Fpackage.json%3A13%2C33&workspaceId=836800c1-9711-491c-a89b-3ac24cbd8cd8
What has been done
I copy pasted the victory-vendor script and adapted it such that it includes all the d3 lib we need (colors, interpolation, scale)
I did not used directly the victory-vendor package because they use the legacy d3-voronoid and we are using the new d3-delaunay.
The d3-delaunay is a wrapper on top of delaunator which relies on robust-predicates. This last one seems to also be only ESM so I added both of them to the script such that they are correctly imported for CJS
TODO if we agree on the strategy
- Clean the Licence, and meta information in the package.json
- Make sure the built package only contains the needed stuff
- Make sure release is working fine
- Update intalation documentation
Deploy preview: https://deploy-preview-13608--material-ui-x.netlify.app/
Updated pages:
Generated by :no_entry_sign: dangerJS against eb8d06b19c48e9172f3f91ad1a437e4118ade3e7
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
This pull request has conflicts, please resolve those before we can evaluate the pull request.
@LukasTy My quick computation of yesterday (10 libraries in 5 folders = around 50 files) was wrong because I forgot the 450 @babel files 🙈
Are you still ok to add this in the repo? For me the deal is still ok since those files should not be modified except when bumping d3 version
TIL: You need to run the following command to ensure your codeov config is valid.
Our config was not valid, because the ignore can not be that deep in the config.
curl --data-binary @codecov.yml https://codecov.io/validate
I'd also be interested in @JCQuintas opinion as this path was his idea if I remember correctly. 🤔
I think it was @flaviendelangle
This pull request has conflicts, please resolve those before we can evaluate the pull request.
vale github action failing is expected.
It fails when to many lines are modified, because that crash the review dog (which usually put comment in docs update)
This pull request has conflicts, please resolve those before we can evaluate the pull request.
I didn't take the time to test if the latest changes still fix the mentioned issues. 🙈
No wories, I plan to do it before merging, but after having CI green :)
It only occurred to me now, but couldn't we have bundled the lib files into a single file for cjs? 🤔 Probably for a future iteration though 😆
couldn't we have bundled the lib files into a single file for cjs? 🤔 Probably for a future iteration though 😆
We could have done that, yes. 👍
But we tried keeping the solution close to victory-vendor AFAIK. 🤔
Good idea. I keep it in case the number of file generated reveals to be an issue :+1:
Finally managed to take a look at this. Some remarks:
- Looks good! I see nothing in the package layout that wouldn't work. It's just re-exporting for ESM so no resolution problems there.
- Rollup would likely have been more appropriate here as a tool to bundle those d3 sources. It would completely eliminate the custom resolver required in babel. (If you find that resolving is required in babel, it's an indication that a bundler should be used instead). But it's working well, we can roll with it 👍.
- Ideally the output extensions of the esm target are
.mjs