-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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-infra] Add recursively the relative modules in the demos #44150
Conversation
Netlify deploy previewhttps://deploy-preview-44150--material-ui.netlify.app/ Bundle size report |
content: curr.raw, | ||
content: flattenRelativeImports( | ||
curr.raw, | ||
demoData.relativeModules?.map((file) => file.module), |
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.
Do the same as the Demo.js file as we now allow relatives imports in all files.
@@ -5,6 +5,9 @@ export default function flattenRelativeImports(rawCode: string, modulePaths: str | |||
// Move the relative import to the current directory | |||
const newPath = `./${pathWithoutExtension.replace(/^.*[\\/]/g, '')}`; | |||
newCode = newCode.replace(pathWithoutExtension, newPath); | |||
|
|||
// Replace the relative import towards parent directory | |||
newCode = newCode.replace('../', './'); |
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 replace nested imports toward the parent directory, e.g. a file importing from "../utils". Not sure if this is the best way to fix this, but I am open for suggestions.
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 got confused when writing this piece of code. I thaught path
would be the path as expressed in the file. But it's the path from the root of the Demo.
-
We could move the
replace
outside of theforEach
by using areplaceAll
. Currently, it works because we apply this Replace once per file existing in the demo -
Another option would be to use similar Regexp as the one to compute
newPath
newCode = newCode.replace(/from '.*[\\/]/g, `from './`);
It replaces any sequence of .
and /
to support multiple levels of nested components (I hope we
will not reach that level 🙈)
And the from '
prevent replacing a ../
that could be present in the comments
@@ -63,7 +63,10 @@ function createReactApp(demoData: DemoData) { | |||
(acc, curr) => ({ | |||
...acc, | |||
// Remove the path and keep the filename | |||
[`${curr.module.replace(/^.*[\\/]/g, '')}`]: curr.raw, | |||
[`${curr.module.replace(/^.*[\\/]/g, '')}`]: flattenRelativeImports( |
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.
Same change as in the CodeSandbox file.
@@ -198,6 +198,7 @@ module.exports = async function demoLoader() { | |||
} | |||
} | |||
} | |||
return relativeModuleFilename; |
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 decided to update the method to return the file as it is needed when iterating the relative modules.
} catch { | ||
throw new Error( | ||
`Could not find a module for the relative import "${relativeModuleID}" in the demo "${demoName}"`, | ||
for (const variant of variants) { |
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 only iterates through ['JS', 'TS']
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 left few comments, but for me it's good.
I just wait for the removal of the test demo before approving to be sure nobody merge it by mistake 🙈
@@ -5,6 +5,9 @@ export default function flattenRelativeImports(rawCode: string, modulePaths: str | |||
// Move the relative import to the current directory | |||
const newPath = `./${pathWithoutExtension.replace(/^.*[\\/]/g, '')}`; | |||
newCode = newCode.replace(pathWithoutExtension, newPath); | |||
|
|||
// Replace the relative import towards parent directory | |||
newCode = newCode.replace('../', './'); |
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 got confused when writing this piece of code. I thaught path
would be the path as expressed in the file. But it's the path from the root of the Demo.
-
We could move the
replace
outside of theforEach
by using areplaceAll
. Currently, it works because we apply this Replace once per file existing in the demo -
Another option would be to use similar Regexp as the one to compute
newPath
newCode = newCode.replace(/from '.*[\\/]/g, `from './`);
It replaces any sequence of .
and /
to support multiple levels of nested components (I hope we
will not reach that level 🙈)
And the from '
prevent replacing a ../
that could be present in the comments
@alexfauquette all comments were resolved in separate commits, you can checke the changes commit per commit. Everything was working as expected (I've tested after all the changes). I noticed one breaking change tough, I described it in the PR description. I am tagging @bharatkashyap too to be aware for it for the Toolpad project. Tagging also @michaldudak I am not sure if this affects the new Base UI docs, but just in case. |
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.
Nice reduction of the flattenRelativeImports
👍
BY the way @bharatkashyap this PR might close some of the followup issues
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.
What a great improvement! Thanks for working on this. Added some small suggestions
For the record, the demo loader used in Base UI docs has this functionality built-in already, so once we propagate it to other repos, it should still work fine. |
Co-authored-by: Bharat Kashyap <[email protected]> Signed-off-by: Marija Najdova <[email protected]>
BRREAKING CHANGE
For the demos that used relative imports, make sure that you have a TypeScript version of the relative modules too, as the infra was updated to look for TypeScript files in the TypeScript version of the demo. In Core, the only usage was in the Autocomplete component related to the
top100films.js
file. You can likely easily search for this in thedocs/data
directory of your projects.This PR adds support for complex demos that need a specific folder structure in order to improve the collocation of the files in the demos. I copied the Advanced ListView demo from MUI X.
List of needed changes to enable this feature:
I've tested that:
TODO: