[code-infra] Skip codesandbox iframe demos in regressions tests #18970
Conversation
| @@ -36,6 +36,7 @@ | |||
| "react-dom": "catalog:", | |||
| "react-router": "catalog:", | |||
| "react-transition-group": "catalog:", | |||
| "tsx": "catalog:" | |||
| "tsx": "catalog:", | |||
| "vite": "catalog:" | |||
There was a problem hiding this comment.
Added vite as dependency, it's used directly in the build script
|
Deploy preview: https://deploy-preview-18970--material-ui-x.netlify.app/ Bundle size report
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
| @@ -119,6 +121,11 @@ async function main() { | |||
| return; | |||
| } | |||
|
|
|||
| if (codesandboxIframeDemos.some((demo) => route.includes(demo))) { | |||
| // Ignore codesandbox embedded demos since they're not using packages built from the branch anyway. | |||
| return; | |||
There was a problem hiding this comment.
I'm surprised to see all these blacklist patterns here. I thought we completely switched to negative globs in https://github.com/mui/mui-x/blob/master/test/regressions/testsBySuite.ts. Those are much better as they just avoid bundling the unused demos altogether, which is better for performance.
Signed-off-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
Signed-off-by: Andrew Cherniavskii <andrew.cherniavskii@gmail.com>
| "vite": "^7.0.6", | ||
| "vite": "catalog:", | ||
| "vitest": "catalog:", |
There was a problem hiding this comment.
I'm confused on this one. I would have expected the opposite move:
| "vite": "^7.0.6", | |
| "vite": "catalog:", | |
| "vitest": "catalog:", | |
| "vite": "^7.0.6", | |
| "vitest": "^3.2.4", |
To what I understand, catalog is for dependencies, where we can't have one package at the root's package.json. In the case of devDependencies, we can, so catalog only add indirections, so should be removed?
There was a problem hiding this comment.
Ah, you mean that we don't care about having different versions of dev dependencies?
IDK, perhaps, but wouldn't dependatabot still try to update bump them to the latest version anyway?
There was a problem hiding this comment.
Ah, you mean that we don't care about having different versions of dev dependencies?
I imagine that if a dev dependency is shared between packages, we can have it set at the root package.json, in which case, we can't have multiple version.
There was a problem hiding this comment.
we can have it set at the root package.json
This was the case before this PR, but the build script in test/regressions didn't see it, so I had to declare it there too: #18970 (comment)
There was a problem hiding this comment.
but the build script in test/regressions didn't see it
Have to agree that in the past I also have seen vite particularly act strange sometimes if it is installed at the workspace root.
Advantages of declaring at he root:
- single place to declare dev dependencies, naturally kept in sync across workspaces
Advantages of declaring at the package level
- packages remain self-contained units. i.e. they can be copied between monorepos and still work. Not that you necessarily do this often in practice, but as a principle it establishes the package boundary as very strict.
- dev dependencies that are only meant for specific packages are not importable by all.
- Some tooling may not expect to be placed at the root of a monorepo and act strange.
Under pnpm, for both solutions, the overhead should be minimal
My personal opinion:
I think we should install tooling exactly where you want to run it from. i.e. vitest, eslint, prettier,... from the monorepo root, but when vite is being run fro the package itself, install it there.
I'm not a huge fan of catalog: dependencies. It creates a system of defining dependency ranges that does not port well between package managers, and therefore is not expected to be well supported by tooling. I think it would be more appropriate to rely on renovatebot to update packages in sync, and to rely on linting (e.g. syncpack, or our own) to guard against manual mishaps.
| @@ -24,9 +24,18 @@ const docsImports = import.meta.glob<React.ComponentType>( | |||
| '!docsx/data/date-pickers/date-calendar/DateCalendarServerRequest', // Has random behavior (TODO: Use seeded random) | |||
| '!docsx/data/charts/tooltip/Custom*', // Composition example | |||
| '!docsx/data/charts/tooltip/Item*', // Composition example | |||
| '!docsx/data/charts/tooltip/AxisFormatter', | |||
There was a problem hiding this comment.
Are we trying to move away completely from the "Exclusions" pattern, to only use the NoSnap suffix pattern?
There was a problem hiding this comment.
I don't know why these didn't have NoSnap in their filenames, I just reflected here the same logic that was already present in test/regressions/index.test.ts.
But perhaps NoSnap suffix would be a bit better because it's more explicit.
cc @mui/xcharts
There was a problem hiding this comment.
All those demo are related to the Tooltip which by default does not show up in screenshot (you need to interact with the chart) so all the charts/tooltip/ demo are kind of useless for argos
These screenshots are empty anyway (we don't wait until they load), and they're not using packages built from the branch.