[code-infra] Optimize checkMaterialVersion
#20307
Conversation
Janpot
added
test
type: enhancement
|
Deploy preview: https://deploy-preview-20307--material-ui-x.netlify.app/ Bundle size report
|
Janpot
requested review from
alexfauquette and
bernardobelchior
as code owners
| defineConfig({ | ||
| test: { | ||
| name: getTestName(import.meta.url), | ||
| exclude: ['**/materialVersion.test.tsx'], |
There was a problem hiding this comment.
Cant we put this in vitest.config.mts based on process.env.BROWSER?
There was a problem hiding this comment.
imo we shouldn't use this variable for anything else than moving config from cli to your script. as soon as it is read and parsed, use that js value and distribute with regular JavaScript constructs. better to create a factory function for your config and pass a browser boolean to it. this is ensures all logic is encoded in regular program flow and is easier to debug and reason about. if you disagree let me know, otherwise I'll look into it in a separate PR
There was a problem hiding this comment.
Yeah, you can do whatever with the variable, my point was more to move the logic to single place.
This reverts commit 789d239.
3 participants
Locally, these were some of the slowest tests. There are few problems with them:
catalog:version was wrong. We corrected the behavior for babel runtime in our build tooling, bringing that fix here.catalog:is a version range and it's set in the pnpm config. We should just fetch that version range from the catalog and pretend that's what was set in package.json. No need to resolve it if we also don't do that for the noncatalog:variant of this test.npmin one of its commands instead ofpnpmpnpm installhas been run or something? Personally I'd remove this test altogether, but with these fixes it should be fast enough so up to the team.execaand blowing upLocally I had 9 of those tests in my top-20 leaderboard of slowest tests, after these changes there were none. Overall time went down by 18s but I'm, not sure how stable the overall runtime is