-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[charts] Use vendor to have CJS working out of the box #13608
Conversation
Deploy preview: https://deploy-preview-13608--material-ui-x.netlify.app/ Updated pages: |
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. |
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. |
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.
LGTM. 👍
Leaving some final comments and noting that I didn't take the time to test if the latest changes still fix the mentioned issues. 🙈
} | ||
}, | ||
"dependencies": { | ||
"@babel/runtime": "^7.24.8", |
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 putting it into peerDependencies
instead make any difference? 🤔
Given that it is always going to be used in tandem with mui/x-charts
, which has a direct @babel/runtime
dependency, it should be "fine".
However, if it doesn't change anything in the build process—there is probably no reason to do it. 🤔
@@ -23,6 +24,7 @@ | |||
"@mui/x-date-pickers": "packages/x-date-pickers/build", | |||
"@mui/x-date-pickers-pro": "packages/x-date-pickers-pro/build", | |||
"@mui/x-charts": "packages/x-charts/build", | |||
"@mui/x-charts-vendor": "packages/x-charts-vendor", |
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.
Will deploy examples work without it by using the latest
version?
package.json
Outdated
@@ -65,7 +64,8 @@ | |||
"release:publish:dry-run": "pnpm publish --recursive --tag latest --registry=\"http://localhost:4873/\"", | |||
"release:tag": "node scripts/releaseTag.mjs", | |||
"validate": "concurrently \"pnpm prettier && pnpm eslint\" \"pnpm proptypes\" \"pnpm docs:typescript:formatted\" \"pnpm docs:api\"", | |||
"clean:node_modules": "rimraf --glob \"**/node_modules\"" | |||
"clean:node_modules": "rimraf --glob \"**/node_modules\"", | |||
"build:charts:vendor": "node ./packages/x-charts-vendor/scripts/build.js" |
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.
Is there a point in having this script if we can run pnpm --filter @mui/x-charts-vendor build
or at least use it here? 🤔
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.
I was not aware of this --filter
option
.circleci/config.yml
Outdated
- run: | ||
name: '`pnpm build:charts:vendor` changes committed?' | ||
command: | | ||
pnpm build:charts:vendor |
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.
Nitpick: I see that it takes ~9 seconds on CI, but still, have you considered running it less frequently?
Maybe use the same logic as for the dedupe
?
Lines 126 to 134 in ad605ff
name: '`pnpm dedupe` was run?' | |
command: | | |
# #default-branch-switch | |
if [[ $(git diff --name-status master | grep pnpm-lock) == "" ]]; | |
then | |
echo "No changes to dependencies detected. Skipping..." | |
else | |
pnpm dedupe --check | |
fi |
No wories, I plan to do it before merging, but after having CI green :) |
Co-authored-by: Lukas Tyla <[email protected]> Signed-off-by: Alexandre Fauquette <[email protected]>
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 😆 |
We could have done that, yes. 👍 |
Good idea. I keep it in case the number of file generated reveals to be an issue 👍 |
Finally managed to take a look at this. Some remarks:
|
"version": "7.8.0", | ||
"description": "Vendored dependencies for MUI X Charts", | ||
"author": "MUI Team", | ||
"main": "./index.js", |
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.
This file doesn't exist:
https://publint.dev/@mui/[email protected]
I think that we can remove it, it will be clearer:
"main": "./index.js", |
Edit: fixed in #14465
Signed-off-by: Alexandre Fauquette <[email protected]> Co-authored-by: Lukas Tyla <[email protected]>
Related issues
Would fix [charts][ESM]
@mui/x-charts
does not work with jest #11568 -: with the following modification the error is gone test-victory-vendor alexfauquette/mui-x-charts-jest-issue-minimal-reproduction#1For [charts][ESM] Can't import @mui/x-charts under node.js #11016 we still have some import issues: 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)
Fix [charts][ESM] Doesn't build in Next.js due to require() of ES Module (ERR_REQUIRE_ESM) #9826
https://codesandbox.io/p/devbox/elegant-forest-hvn8fk?file=%2Fnext.config.js%3A7%2C42&workspaceId=d214a4af-c616-40eb-ac12-807102cf00db
Allows to integrate charts unit test with others since the babel hack is not needed anymore
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 use directly the
victory-vendor
package because they use the legacyd3-voronoid
and we are using the newd3-delaunay
.The
d3-delaunay
is a wrapper on top ofdelaunator
which relies onrobust-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 CJSOpen question
Is it a breaking change? I don't think so, because if users are working with ESM this vendor should be transparent. And if they were transpiling d3 modules now they don't need it because we do it for them.
TODO if we agree on the strategy
Changelog
📦 [charts] Use vendor to have CJS working out of the box
This modifies how the package imports D3.js. It should not impact you.
For more context, the initial issue is caused by D3 only exporting ESM. The solution up until now was to export charts with only ESM. But some frameworks are confused by this configuration. So the time we can fix our build, we use a vendor package providing a CJS version of D3.