-
-
Notifications
You must be signed in to change notification settings - Fork 32.8k
[infra] Fix JSON files not being imported in TS demos #47000
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
[infra] Fix JSON files not being imported in TS demos #47000
Conversation
Netlify deploy previewhttps://deploy-preview-47000--material-ui.netlify.app/ Bundle size report
|
packages/markdown/loader.mjs
Outdated
| // If the moduleID does not end with an extension, or ends with an unsupported extension (e.g. ".styling") we need to resolve it | ||
| // Fastest way to get a file extension, see: https://stackoverflow.com/a/12900504/ | ||
| const importType = importModuleID.slice( | ||
| (Math.max(0, importModuleID.lastIndexOf('.')) || Infinity) + 1, | ||
| ); | ||
| const importType = getExtension(importModuleID); |
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.
You copy paste the comments. Maybe keeping is only at one place is enough
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.
Ah, yes, forgot to remove it 👍
| ); | ||
|
|
||
| // the file has already been processed | ||
| if (addedModulesRelativeToModulePath.has(relativeModuleFilePath)) { |
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.
Not 100% sure, but wouldn't it be solved if we just make addedModulesRelativeToModulePath work per-variant?
demos[demoName].relativeModules = {};
}
- const addedModulesRelativeToModulePath = new Set();
await Promise.all(
Array.from(relativeModules.get(demoName)).map(async ([relativeModuleID, variants]) => {
for (const variant of variants) {
+ const addedModulesRelativeToModulePath = new Set();
let raw = '';
const relativeModuleFilePath = path.join(
path.dirname(moduleFilepath),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.
Seems to fix the issue 👍 thanks!
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.
Actually, on a second look, that doesn't seem to be equivalent. Maybe we need to
const addedModulesRelativeToModulePath = { TS: new Set(), JS: new Set() };?
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.
Actually, on a second look, that doesn't seem to be equivalent. Maybe we need to
const addedModulesRelativeToModulePath = { TS: new Set(), JS: new Set() };?
How would that solve the issue? If the TS key is only read when the variant is TS, isn't it equivalent?
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.
Because with the first solution I propose, it would stop deduplicating altogether, addedModulesRelativeToModulePath would never get more than one item added. But if I understand correctly, we want deduplication per variant. So we need two sets instead of one, and add to the set of the current variant we're working on.
I guess, I don't know this code too well neither. Just learning along reviewing 🙂
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.
Ah, you're right. We're iterating over the modules and inside that iteration we iterate over the variants. It would only work it if was the other way around.
08d4459 to
78effb6
Compare
Janpot
left a comment
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.
Justa small suggestion, but at your own discretion
Fixes mui/mui-x#19749.
Fix JSON files not being imported in TS demos.
I'm not very experienced with docs-infra, so feel free to suggest improvements.
It seemed that once a file is processed it won't be processed again. Since JS processing happens before TS processing, modules that independent from the variant won't show up in the TS demo because they will have been processed by the JS demo already.
Before:
Screen.Recording.2025-09-29.at.10.51.34.mov
After:
Screen.Recording.2025-09-29.at.10.51.14.mov