mui-x icon indicating copy to clipboard operation
mui-x copied to clipboard

[charts] Use vendor to have CJS working out of the box

Open alexfauquette opened this issue 1 year ago • 3 comments
trafficstars

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

alexfauquette avatar Jun 24 '24 09:06 alexfauquette

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

Updated pages:

Generated by :no_entry_sign: dangerJS against eb8d06b19c48e9172f3f91ad1a437e4118ade3e7

mui-bot avatar Jun 24 '24 09:06 mui-bot

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

github-actions[bot] avatar Jun 25 '24 08:06 github-actions[bot]

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

github-actions[bot] avatar Jun 26 '24 12:06 github-actions[bot]

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

github-actions[bot] avatar Jul 02 '24 19:07 github-actions[bot]

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

github-actions[bot] avatar Jul 05 '24 15:07 github-actions[bot]

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

github-actions[bot] avatar Jul 16 '24 12:07 github-actions[bot]

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

github-actions[bot] avatar Jul 22 '24 15:07 github-actions[bot]

@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

alexfauquette avatar Jul 23 '24 07:07 alexfauquette

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

alexfauquette avatar Jul 23 '24 14:07 alexfauquette

I'd also be interested in @JCQuintas opinion as this path was his idea if I remember correctly. 🤔

I think it was @flaviendelangle

alexfauquette avatar Jul 24 '24 07:07 alexfauquette

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

github-actions[bot] avatar Jul 24 '24 10:07 github-actions[bot]

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)

alexfauquette avatar Jul 26 '24 09:07 alexfauquette

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

github-actions[bot] avatar Jul 26 '24 09:07 github-actions[bot]

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 :)

alexfauquette avatar Jul 26 '24 13:07 alexfauquette

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 😆

JCQuintas avatar Jul 29 '24 19:07 JCQuintas

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. 🤔

LukasTy avatar Jul 30 '24 08:07 LukasTy

Good idea. I keep it in case the number of file generated reveals to be an issue :+1:

alexfauquette avatar Jul 30 '24 09:07 alexfauquette

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

Janpot avatar Aug 29 '24 09:08 Janpot