-
-
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
Changes from 23 commits
ae7a4a3
c46b401
0d45b9c
eaf8c8b
49f743c
94f6877
c74279a
bc2f212
ebf7b12
0a35124
7bde9b5
140c7e1
4e3fa69
f8fb844
11eb62a
3c9a4ab
4f4a121
f354771
3310079
58042e8
fb9586d
563bd0f
159cdc6
a0c66e9
41fdb7d
8ed7c37
aa66988
b7ffe8e
0d72a58
200eb1b
71d9cf3
e705edd
64e5802
683e05f
eb50d4f
b59002e
f35af6c
800b99f
df2f570
df37a6f
ec274ec
1e1ec4a
a9f0102
c5f4849
3ab5222
f70d698
fca1af4
1055b74
cc68da2
38e222b
d7db24a
f975545
a8aa2c0
eb8d06b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -91,6 +91,9 @@ commands: | |
- run: | ||
name: Install js dependencies | ||
command: pnpm install | ||
- run: | ||
name: Build charts vendor | ||
command: node ./packages/x-charts-vendor/scripts/build.js | ||
- when: | ||
condition: << parameters.browsers >> | ||
steps: | ||
|
@@ -128,9 +131,6 @@ jobs: | |
steps: | ||
- checkout | ||
- install_js | ||
- run: | ||
name: Tests charts | ||
command: pnpm test:charts:unit # Run special test for charts due to ESM compatibility issue | ||
- run: | ||
Comment on lines
139
to
140
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thos test are now in unit test since it does not require any specific babel config |
||
name: Tests fake browser | ||
command: pnpm test:coverage | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,7 @@ | |
"packages/x-date-pickers", | ||
"packages/x-date-pickers-pro", | ||
"packages/x-charts", | ||
"packages/x-charts-vendor", | ||
"packages/x-tree-view", | ||
"packages/x-internals" | ||
], | ||
|
@@ -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 commentThe reason will be displayed to describe this comment to others. Learn more. Not sure it's usefull to ask circle ci to build it 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will deploy examples work without it by using the |
||
"@mui/x-charts-pro": "packages/x-charts-pro/build", | ||
"@mui/x-tree-view": "packages/x-tree-view/build", | ||
"@mui/x-tree-view-pro": "packages/x-tree-view-pro/build", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ netlify/functions | |
/docs/pages/playground/ | ||
/lerna.json | ||
/packages/x-codemod/src/**/*.spec.js | ||
/packages/x-charts-vendor | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Except the script all the files are automatically generated, so I just skipped the entire folder There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would argue it is useful for the script itself to be linted 😅 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a copy past of victory-vendor so it nearly respect non of our linting rules. But could do it in a cleanup phase |
||
build | ||
CHANGELOG.md | ||
dist | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,7 +5,7 @@ | |
"author": "MUI Team", | ||
"license": "MIT", | ||
"scripts": { | ||
"build": "rimraf ./export && cross-env NODE_ENV=production next build --profile && pnpm build-sw", | ||
"build": "rimraf ./export && node ../packages/x-charts-vendor/scripts/build.js && cross-env NODE_ENV=production next build --profile && pnpm build-sw", | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Build the vendor package otherwise netlify has no access to it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This currently means all our devs need to build the lib before we can run There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I assumed that once merged, everybody would run the build, and then it's done since all the generated files are in .gitignore But effectively it's not friendly for external contributors. Maybe adding a parameter such that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, maybe check if the entrypoints are present and run the script otherwise? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also breaks when running tests btw |
||
"build:clean": "rimraf .next && pnpm build", | ||
"build-sw": "node ./scripts/buildServiceWorker.js", | ||
"dev": "next dev --port 3001", | ||
|
@@ -37,6 +37,7 @@ | |
"@mui/system": "^5.15.20", | ||
"@mui/utils": "^5.15.20", | ||
"@mui/x-charts": "workspace:*", | ||
"@mui/x-charts-vendor": "workspace:*", | ||
"@mui/x-data-grid": "workspace:*", | ||
"@mui/x-data-grid-generator": "workspace:*", | ||
"@mui/x-data-grid-premium": "workspace:*", | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||
/** | ||||
* Transform d3 ESM libraries to vendored CommonJS libraries | ||||
* | ||||
* This produces `lib-vendor/d3-<package name>/src` files that have | ||||
* internally consistent references to other d3 packages. It is only meant | ||||
* to be used for the CommonJS import path. | ||||
*/ | ||||
const path = require('path'); | ||||
|
||||
module.exports = { | ||||
only: ['node_modules/*/src/**/*.js', 'node_modules/*/**/*.js'], | ||||
plugins: [ | ||||
[ | ||||
'@babel/transform-modules-commonjs', | ||||
{ | ||||
strict: false, | ||||
allowTopLevelThis: true, | ||||
}, | ||||
], | ||||
[ | ||||
'module-resolver', | ||||
{ | ||||
// Convert all imports for _other_ d3 dependencies to the relative | ||||
// path in our vendor package. | ||||
resolvePath(sourcePath, currentFile) { | ||||
const d3pattern = | ||||
/^(?<pkg>(d3-[^\/]+|internmap|delaunator|robust-predicates))(?<path>.*)/; | ||||
const match = d3pattern.exec(sourcePath); | ||||
if (match) { | ||||
// We're assuming a common shape of d3 packages: | ||||
// - Only top level imports "d3-<whatever>" | ||||
// - With no path components (like "d3-<whatever>/path/to.js") | ||||
if (match.groups.path) { | ||||
throw new Error(`Unable to process ${sourcePath} import in ${currentFile}`); | ||||
} | ||||
|
||||
// Get Vendor package path. | ||||
const vendorPkg = ['delaunator', 'robust-predicates'].includes(match.groups.pkg) | ||||
? path.resolve(__dirname, `./lib-vendor/${match.groups.pkg}/index.js`) | ||||
: path.resolve(__dirname, `./lib-vendor/${match.groups.pkg}/src/index.js`); | ||||
Comment on lines
+38
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here is the main difference with the original script.
So I added the to the vendor too. Those lines are here because they do not have the exact same structure as the d3 packages |
||||
|
||||
console.log({ vendorPkg }); | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||
// Derive relative path to vendor lib to have a file like move from: | ||||
// - 'node_modules/d3-interpolate/src/rgb.js' | ||||
// - 'lib-vendor/d3-interpolate/src/rgb.js' | ||||
// and have an import transform like: | ||||
// - `d3-color` | ||||
// - `../../d3-color` | ||||
const currentFileVendor = currentFile.replace(/^node_modules/, 'lib-vendor'); | ||||
const relPathToPkg = path | ||||
.relative(path.dirname(currentFileVendor), vendorPkg) | ||||
.replace(/\\/g, '/'); | ||||
|
||||
return relPathToPkg; | ||||
} | ||||
|
||||
return sourcePath; | ||||
}, | ||||
}, | ||||
], | ||||
], | ||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
/lib-vendor | ||
/d3-* | ||
/delaunator.* | ||
/robust-predicates.* | ||
/internmap.js | ||
/es | ||
/lib |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,72 @@ | ||
# VictoryVendor | ||
|
||
Vendored dependencies for Victory. | ||
|
||
## Background | ||
alexfauquette marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
D3 has released most of its libraries as ESM-only. This means that consumers in Node.js applications can no longer just `require()` anything with a d3 transitive dependency, including much of Victory. | ||
|
||
To help provide an easy path to folks still using CommonJS in their Node.js applications that consume Victory, we now provide this package to vendor in various d3-related packages. | ||
|
||
## Packages | ||
|
||
We presently provide the following top-level libraries: | ||
|
||
<!-- cat packages/victory-vendor/package.json | egrep '"d3-' | egrep -o 'd3-[^"]*'| sor t--> | ||
|
||
- d3-ease | ||
- d3-interpolate | ||
- d3-scale | ||
- d3-shape | ||
- d3-timer | ||
|
||
This is the total list of top and transitive libraries we vendor: | ||
|
||
<!-- ls packages/victory-vendor/lib-vendor | sort --> | ||
|
||
- d3-array | ||
- d3-color | ||
- d3-ease | ||
- d3-format | ||
- d3-interpolate | ||
- d3-path | ||
- d3-scale | ||
- d3-shape | ||
- d3-time | ||
- d3-time-format | ||
- d3-timer | ||
- internmap | ||
|
||
Note that this does _not_ include the following D3 libraries that still support CommonJS: | ||
|
||
- d3-voronoi | ||
|
||
## How it works | ||
|
||
We provide two alternate paths and behaviors -- for ESM and CommonJS | ||
|
||
### ESM | ||
|
||
If you do a Node.js import like: | ||
|
||
```js | ||
import { interpolate } from '@mui/x-charts-vendor/d3-interpolate'; | ||
``` | ||
|
||
under the hood it's going to just re-export and pass you through to `node_modules/d3-interpolate`, the **real** ESM library from D3. | ||
|
||
### CommonJS | ||
|
||
If you do a Node.js import like: | ||
|
||
```js | ||
const { interpolate } = require('@mui/x-charts-vendor/d3-interpolate'); | ||
``` | ||
|
||
under the hood it's going to will go to an alternate path that contains the transpiled version of the underlying d3 library to be found at `victory-vendor/lib-vendor/d3-interpolate/**/*.js`. This futher has internally consistent import references to other `victory-vendor/lib-vendor/<pkg-name>` paths. | ||
|
||
Note that for some tooling (like Jest) that doesn't play well with `package.json:exports` routing to this CommonJS path, we **also** output a root file in the form of `victory-vendor/d3-interpolate.js`. | ||
|
||
## Licenses | ||
|
||
This project is released under the MIT license, but the vendor'ed in libraries include other licenses (e.g. ISC) that we enumerate in our `package.json:license` field. | ||
LukasTy marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||
{ | ||||
"name": "@mui/x-charts-vendor", | ||||
"version": "7.8.0", | ||||
"description": "Vendored dependencies for MUI charts", | ||||
LukasTy marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
"author": "MUI Team", | ||||
"main": "./index.js", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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:
Suggested change
Edit: fixed in #14465 |
||||
"keywords": [ | ||||
"data visualization", | ||||
"React", | ||||
"d3", | ||||
"charting" | ||||
], | ||||
"repository": { | ||||
"type": "git", | ||||
"url": "https://github.com/mui/mui-x" | ||||
}, | ||||
"license": "MIT AND ISC", | ||||
"exports": { | ||||
"./package.json": "./package.json", | ||||
"./d3-*": { | ||||
"types": "./d3-*.d.ts", | ||||
"import": "./es/d3-*.js", | ||||
"default": "./lib/d3-*.js" | ||||
} | ||||
JCQuintas marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
}, | ||||
"dependencies": { | ||||
"@babel/runtime": "^7.24.7", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Am I missing something? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For a reason I ignore, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed all our packages have the So I went the same with the vendor such that helpers are not inlined in the generated code. Will see if it solves the regression test |
||||
"d3-color": "^3.1.0", | ||||
"d3-delaunay": "^6.0.4", | ||||
"d3-interpolate": "^3.0.1", | ||||
"d3-scale": "^4.0.2", | ||||
"d3-shape": "^3.2.0", | ||||
"d3-time": "^3.1.0", | ||||
"@types/d3-color": "^3.1.3", | ||||
"@types/d3-delaunay": "^6.0.4", | ||||
"@types/d3-interpolate": "^3.0.4", | ||||
"@types/d3-scale": "^4.0.8", | ||||
"@types/d3-shape": "^3.1.6", | ||||
"@types/d3-time": "^3.0.3", | ||||
"robust-predicates": "^3.0.2", | ||||
"delaunator": "^5.0.1" | ||||
}, | ||||
"devDependencies": { | ||||
"d3-format": "^3.1.0", | ||||
"d3-time-format": "^4.1.0", | ||||
"d3-path": "^3.0.1", | ||||
"d3-array": "^3.1.6", | ||||
"@types/d3-format": "^3.0.4", | ||||
"@types/d3-time-format": "^4.0.3", | ||||
"@types/d3-path": "^3.0.1", | ||||
"@types/d3-array": "^3.0.3", | ||||
"internmap": "^2.0.3", | ||||
"execa": "^6.1.0", | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tried using a newer version of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah I would assume all of |
||||
"rimraf": "^3.0.2" | ||||
}, | ||||
"publishConfig": { | ||||
"provenance": true | ||||
}, | ||||
"scripts": { | ||||
"build": "node ./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.
We need to build the package each time. Otherwise the folder is empty and so every script complains that the package does not exist
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.
Should we provide the package built already instead of having to build it in the pipelines and docs?
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 would create an inconsistency in the release:
@mui/x-charts-vendor
is not published so no preview could work@mui/x-charts-vendor
which is not practical if you need to bump one of the d3-lib