-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[docs] Fix charts demo using too deep import #10263
Conversation
Netlify deploy previewNetlify deploy preview: https://deploy-preview-10263--material-ui-x.netlify.app/ Updated pagesNo updates. These are the results for the performance tests:
|
Not sure if related, but bumping into mui/material-ui#38860 while debugging this. After I relocate the tsconfig.json, I can't seem to replicate the behavior of this PR locally. @LukasTy Did you manage to? |
Do you mean if I'm able to reproduce the problem(s) locally? Then no, it's evident only in codesandbox. 🤔 |
When you use the "./hooks/*": {
"types": "./hooks/*/index.d.ts",
"require": "./hooks/*/index.js",
"import": "./esm/hooks/*/index.js"
} If these subpaths are not intended to be public API, then I guess we should change the import to import { useDrawingArea } from "@mui/x-charts"; |
Thanks for the input @Janpot . |
@@ -1,6 +1,6 @@ | |||
import * as React from 'react'; | |||
import { PieChart } from '@mui/x-charts/PieChart'; | |||
import { useDrawingArea } from '@mui/x-charts/hooks/useDrawingArea'; |
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 have a lint rule that was probably not set-up for the charts
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 code is in the docs
folder. I don't think that eslint
targets it. 🤔
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.
Yes, they don't have the The reason for the setup is as follows. |
Thank you for providing the insight I was not aware of. 👍 |
packages/x-charts/package.json
Outdated
@@ -71,12 +71,14 @@ | |||
".": { | |||
"types": "./index.d.ts", | |||
"require": "./index.js", | |||
"import": "./esm/index.js" | |||
"import": "./esm/index.js", | |||
"default": "./modern/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.
@LukasTy Is this still necessary? I'm hesitant adding more conditions if they don't solve an actual issue.
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.
Yes, it is the main change that fixed all the demos.
Without it none of the demos load in codesandbox.
The changes on one of the demos is for import depth fixing. 😉
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.
Any idea on why react-scripts
isn't resolving to the import
condition?
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's probably something that codesandbox
is doing on their side, because StackBlitz is working fine. 🤷
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.
Yep, it feels a bit like we're solving the problem in the wrong location. When I fix the codesandbox so that the files are actually covered by the tsconfig.json
I get a different error: https://codesandbox.io/s/peaceful-austin-8ktz7l?file=/tsconfig.json
I have a feeling that could be related to us not adding file extensions to our imports.
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.
Or we could focus on solving #10257?
Maybe solving it could also cover the issue that codesandbox is having. 🤔
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 seems that just removing the require
condition also fixes the issue: #10272
It's a problematic condition because it wouldn't work in the first place. That code is trying to require
a ESM only module (d3
).
Still no clue as to why react-scripts
in this specific codesandbox setup gives precedence to the default
condition though.
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.
Feel free to incorporate #10272 in this PR and merge.
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.
Or maybe just merge your PR specifically targeting the fix you've done, invested time, and resolved? 🤔
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.
Can I get approval to include it in the release? 🤔
Ok for me. But I'd remove the "default"
condition if it's not needed. The ESM landscape is already full of misconfigured setups. Let's keep our chances of adding another one to a minimum 😄
Part of #10195
Adding @Janpot as a preferred reviewer as he approved #10111, which might be related.
Ended up only fixing the demo imports.
The root cause is covered with #10272