[code-infra] Use vale rules from code-infra package#48173
[code-infra] Use vale rules from code-infra package#48173brijeshb42 wants to merge 4 commits intomui:masterfrom
Conversation
Netlify deploy previewhttps://deploy-preview-48173--material-ui.netlify.app/ Bundle size report
|
f840749 to
7028099
Compare
b9b8ffa to
b011ff5
Compare
AGENTS.md
Outdated
| ``` | ||
|
|
||
| ### Testing | ||
| ### Testing JSDOM |
There was a problem hiding this comment.
This was just to check the error reporting. But since the workflow change is in this PR, it only gets read-only token. So vale cant add annotations to the PR.
Once this PR gets merged, annotation will get added inline.
b011ff5 to
c0383e8
Compare
.github/workflows/ci.yml
Outdated
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| id: vale-inputs | ||
| run: | | ||
| echo "files=$(git ls-files '*.md' '*.mdx' | paste -sd ',' -)" >> $GITHUB_OUTPUT |
There was a problem hiding this comment.
If we don't do this, vale traverses the whole directory tree, including node_modules and breaks when it encounters symlinks. Even its glob flag doesnt work.
We could also update this to only consider the changed md files in the PR and not all md files.
run: |
if [ "${{ github.event_name }}" = "pull_request" ]; then
FILES=$(git diff --name-only --diff-filter=ACMR ${{ github.event.pull_request.base.sha }}..HEAD -- '*.md' '*.mdx' | paste -sd ',' -)
else
FILES=$(git ls-files '*.md' '*.mdx' | paste -sd ',' -)
fi
echo "files=$FILES" >> $GITHUB_OUTPUT
echo "version=$(node -p 'require("./package.json").mui.valeVersion')" >> $GITHUB_OUTPUT
package.json
Outdated
| "version": "9.0.0-beta.0", | ||
| "private": true, | ||
| "mui": { | ||
| "valeVersion": "3.12.0" |
There was a problem hiding this comment.
this will not automatically update through renovate. So we can either keep it as-is (update manually or through custom renovate manager) or add @vvago/vale as a devDepedency back insted of the current pnpm dlx based usage.
.github/workflows/ci.yml
Outdated
| # Required, set by GitHub actions automatically: | ||
| # https://docs.github.com/en/actions/security-guides/automatic-token-authentication#about-the-github_token-secret | ||
| token: ${{secrets.GITHUB_TOKEN}} | ||
| version: ${{ steps.vale-inputs.outputs.version }} |
There was a problem hiding this comment.
without flags, it logs warnings as well. So we can pass flags to filter based just on errors similar to what we do in pnpm valelint - vale --filter='.Level==\"error\"'
There was a problem hiding this comment.
Pull request overview
This PR migrates Vale configuration/rules to consume the shared rules shipped from @mui/internal-code-infra, removing the in-repo docs/mui-vale rules bundle and wiring Vale into the main CI workflow.
Changes:
- Point Vale’s
.vale.inito use rules from@mui/internal-code-infrainstead ofdocs/mui-vale.zip. - Remove the
docs/mui-valerule sources and the dedicatedvale-action.ymlworkflow. - Add Vale execution to
.github/workflows/ci.ymland centralize the Vale CLI version inpackage.json.
Reviewed changes
Copilot reviewed 11 out of 13 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
package.json |
Adds a central mui.valeVersion and updates valelint; switches @mui/internal-code-infra to a pkg.pr.new URL. |
pnpm-lock.yaml |
Updates lockfile entries to match the @mui/internal-code-infra source change. |
.vale.ini |
Switches Vale package source from docs/mui-vale.zip to @mui/internal-code-infra. |
.github/workflows/ci.yml |
Runs Vale as part of CI (Ubuntu leg) and adjusts job permissions. |
.github/workflows/vale-action.yml |
Removes the standalone Vale workflow. |
docs/mui-vale/** (multiple deletions) |
Removes the previously vendored MUI Vale rules/styles and related config. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.github/workflows/ci.yml
Outdated
| - name: Collect Vale inputs | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| id: vale-inputs | ||
| run: | | ||
| echo "files=$(git ls-files '*.md' '*.mdx' | paste -sd ',' -)" >> $GITHUB_OUTPUT | ||
| echo "version=$(node -p 'require("./package.json").mui.valeVersion')" >> $GITHUB_OUTPUT | ||
| - name: Vale | ||
| if: ${{ matrix.os == 'ubuntu-latest' }} | ||
| uses: vale-cli/vale-action@d89dee975228ae261d22c15adcd03578634d429c # v2.1.1 | ||
| continue-on-error: true # GitHub Action flag needed until https://github.com/errata-ai/vale-action/issues/89 is fixed |
There was a problem hiding this comment.
The Vale step is added to ci.yml, but this workflow is configured with on.pull_request.paths-ignore: ['docs/**']. As a result, PRs that only touch docs/markdown won’t run this workflow, so Vale won’t run on the changes it’s meant to validate. Consider keeping Vale in a separate workflow that runs on all PRs (or at least on docs/**), or adjusting the CI/CI Check path filters so Vale still executes for docs-only PRs.
package.json
Outdated
| "@mui/internal-babel-plugin-minify-errors": "2.0.8-canary.24", | ||
| "@mui/internal-bundle-size-checker": "1.0.9-canary.70", | ||
| "@mui/internal-code-infra": "0.0.4-canary.11", | ||
| "@mui/internal-code-infra": "https://pkg.pr.new/mui/mui-public/@mui/internal-code-infra@d11ec04", |
There was a problem hiding this comment.
@mui/internal-code-infra is pinned to a pkg.pr.new preview URL. Those preview artifacts are intended for temporary testing and can become unavailable over time, which would break installs and make builds less reproducible. Please switch this back to a published version/range (e.g. the canary you had before) and only use pkg.pr.new overrides locally or in short-lived test branches.
| "@mui/internal-code-infra": "https://pkg.pr.new/mui/mui-public/@mui/internal-code-infra@d11ec04", | |
| "@mui/internal-code-infra": "workspace:^", |
1975681 to
72b7ad0
Compare
| # 3. Run `pnpm docs:zipRules` to generate the zip files | ||
| # 4. You can test locally by replacing the url with the file path of the generated zip | ||
| Packages = Google, docs/mui-vale.zip | ||
| Packages = Google, ./node_modules/@mui/internal-code-infra/vale |
There was a problem hiding this comment.
we can't use a github url, pinned to a commit, and update it together with things like the orb?
There was a problem hiding this comment.
You mean keep docs/mui-vale.zip file and just use raw.github... url?
That can also work. But we should still keep the rules in code-infra and not core repo.
There was a problem hiding this comment.
Coming to think of it, it'll only work if we continue to maintain the zip file directly in the repo (as we do right now). But I am not in favor of keeping zip file, since you have to remember to zip it everytime you make a change to any of the files.
There was a problem hiding this comment.
We could verify that in CI?
idk, reaching in the node modules folder always feels like a red flag to me.
edit: not sure, need to think about this.
There was a problem hiding this comment.
Assuming that we move vale files to code-infra, seems like similar amount of work for both -
- dont zip at all and just download package tgz directly and extract. This has only one CI concern to think of which would be re-usable workflow in
mui-publicrepo and can be triggered from other repos as well.
or
- zip the directory and then verify its hash matches with the existing zip. This now has 2 different CI concerns. One for the actual vale lint (similar to the above) and another CI job in
mui-publicto verify the zip is up to date.
I like the first one better.
There was a problem hiding this comment.
reaching in the node modules folder always feels like a red flag to me
It can be any folder actually where we can extract the vale files to, unless you are talking about reaching into the npm package zip file.
It is just convenient to reach from node_modules because it can run on user system as well as CI without any code change. Also, I don't see any reason of this being a red flag though. It can be considered same as adding explicit export path to package.json.
c85e313 to
5434e8e
Compare
package.json
Outdated
| "stylelint": "stylelint --reportInvalidScopeDisables --reportNeedlessDisables \"docs/**/*.?(c|m)[jt]s?(x)\" \"docs/**/*.css\" --ignore-path .lintignore", | ||
| "markdownlint": "markdownlint-cli2 \"**/*.md\"", | ||
| "valelint": "pnpm dlx --package @vvago/vale vale sync && git ls-files | grep -E \"\\.(md|mdx)$\" | xargs pnpm dlx --package @vvago/vale vale --filter='.Level==\"error\"'", | ||
| "valelint": "pnpm dlx --package @vvago/vale@$(node -p 'require(\"./package.json\").mui.valeVersion') vale sync && git ls-files | grep -E \"\\.(md|mdx)$\" | xargs pnpm dlx --package @vvago/vale@$(node -p 'require(\"./package.json\").mui.valeVersion') vale --filter='.Level==\"error\"'", |
There was a problem hiding this comment.
Another strategy we could think of is add a code-infra vale command, which reads the version from ci.yml, checks the current architecture, downloads the right asset based on that version from https://github.com/vale-cli/vale/releases, (cache in node_modules), and runs it with whatever arguments that were passed.
That way we get
- latest version without needing to wait for
@vvago/vale - no errors on installation when asset can't be fetched
Maybe it'll take us too far for this effort?
There was a problem hiding this comment.
which reads the version from ci.yml
Where will ci have this version specified? In the vale action ? We'll need a renovate regex to update this as well.
Or we can have the version stored in code-infra itself and update that through renovate. Each repo doesn't need to specify their own version, we can control that centrally.
There was a problem hiding this comment.
Where will ci have this version specified? In the vale action ?
Yeah, it has a version input. It was just an idea, we don't have to do it necessarily. I don't know if it would be better than what we have now.
440a1dc to
ae58490
Compare
and ci script to get version from code-infra script
aecd276 to
2bc3d61
Compare
Implements changes in mui/mui-public#1276