Refactor validation tools and update related scripts#77522
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Hi @manzoorwanijk |
|
EDIT: Never mind. I thought this PR is trying to fix that same issue. |
Thanks. I’ll have a look at this soon. |
manzoorwanijk
left a comment
There was a problem hiding this comment.
Thank you for working on this. Let us tidy things a bit.
| "ajv": "8.17.1", | ||
| "chalk": "4.1.1", | ||
| "cross-spawn": "^7.0.6", | ||
| "glob": "7.1.2", | ||
| "jsonc-parser": "3.3.1", | ||
| "simple-git": "3.24.0" |
There was a problem hiding this comment.
Let us use the version ranges here and remove the deps that are not used in the workspace.
| "ajv": "8.17.1", | |
| "chalk": "4.1.1", | |
| "cross-spawn": "^7.0.6", | |
| "glob": "7.1.2", | |
| "jsonc-parser": "3.3.1", | |
| "simple-git": "3.24.0" | |
| "chalk": "^4.1.1", | |
| "glob": "^7.1.2", | |
| "jsonc-parser": "^3.3.1", | |
| "simple-git": "^3.24.0" |
| collectDeps, | ||
| readPackageJson, | ||
| } from '../packages/scripts/utils/license.js'; | ||
| } from '../../packages/scripts/utils/license.js'; |
There was a problem hiding this comment.
Let us add @wordpress/scripts as a dev dependency to this workspace and the use @wordpress/scripts/utils/license.js here.
| "gutenberg", | ||
| "api-docs" | ||
| ], | ||
| "homepage": "https://github.com/WordPress/gutenberg/tree/HEAD/tools/validation/README.md", |
There was a problem hiding this comment.
The README doesn't exist.
| "keywords": [ | ||
| "wordpress", | ||
| "gutenberg", | ||
| "api-docs" |
There was a problem hiding this comment.
This keyword doesn't seem to be relevant.
| "test/unit", | ||
| "tools/api-docs" | ||
| "tools/api-docs", | ||
| "tools/validation" |
There was a problem hiding this comment.
Once you update this branch from trunk, you won't need this change here.
| "lint:lockfile": "npm run validate-package-lock --workspace @wordpress/validation-tools --", | ||
| "lint:tsconfig": "npm run validate-tsconfig --workspace @wordpress/validation-tools --", |
There was a problem hiding this comment.
It's better to have --workspace argument before the other arguments, just in case the script accepts arguments.
| @@ -0,0 +1,42 @@ | |||
| { | |||
| "name": "@wordpress/validation-tools", | |||
| "version": "1.0.0", | |||
There was a problem hiding this comment.
Let us use version 0
| "version": "1.0.0", | |
| "version": "0.0.0", |
|
This needs to be updated from trunk |
- Refactor linting commands in package.json to use the new npm run syntax for workspaces. - Update check-licenses script to import dependencies from @wordpress/scripts. - Change version of @wordpress/validation-tools to 0.0.0 in package.json. - Modify validate-package-lock.js to correctly resolve the package-lock.json path. - Fix tsconfig validation to use JSONC parser for reading tsconfig.json.
Done |
manzoorwanijk
left a comment
There was a problem hiding this comment.
This looks very close. Let us try to reduce the number of changes in check-licenses.mjs to try preserving its git history.
| /* | ||
| * This script checks licenses for production dependencies of packages that are | ||
| * shipped with WordPress (those with wpScript or wpScriptModuleExports in package.json). | ||
| * | ||
| * It works independently of the package manager (npm, pnpm, etc.) by: | ||
| * 1. Reading package.json files to find wpScript packages | ||
| * 2. Reading their production dependencies | ||
| * 3. Resolving each dependency using Node's module resolution | ||
| * 4. Reading the license from each resolved package | ||
| */ |
There was a problem hiding this comment.
Let us not remove this comment please.
Replace bin/packages/validate-typescript-version.js with a generic install-drift check that catches stale node_modules from any dependency, not just typescript. The new script (tools/validation/check-installed-deps.mjs) compares package-lock.json against npm's hidden lockfile (node_modules/.package-lock.json) and verifies installed integrity per registry-fetched entry, ignoring workspace links and platform-skipped optional deps. Set up tools/validation/ as a workspace (@wordpress/validation-tools, picked up by the existing tools/* glob). Build, dev, and build:profile-types invoke the check via `npm run check-installed-deps --workspace @wordpress/validation-tools`. Aligns with the validation-tools workspace direction from #77522 and Also extends the CLI/bin/env eslint override (no-console: off) to cover tools/validation/**, mirroring the same change in #77522.
There was a problem hiding this comment.
We now have tools/validation/package.json in trunk. So let us update the PR from trunk
manzoorwanijk
left a comment
There was a problem hiding this comment.
This is so close. Thank you for the changes.
Let us clean things up a bit before we can merge this
| "bugs": { | ||
| "url": "https://github.com/WordPress/gutenberg/issues" | ||
| }, | ||
| "type": "module", |
There was a problem hiding this comment.
Why are we removing this? If any .js file errs, we can change the extention to .cjs.
There was a problem hiding this comment.
Oh right! I missed that there are two CommonJS files, validate-package-lock.js and check-local-changes.js. It's better to keep "type": "module" and rename the files to .cjs instead. Thanks (Updated after edit)
| "engines": { | ||
| "node": ">=20.10.0" | ||
| }, |
There was a problem hiding this comment.
We don't really need the engines field here because this lives only in the monorepo.
| "check-installed-deps": "node ./check-installed-deps.mjs", | ||
| "check-licenses": "node ./check-licenses.mjs", | ||
| "validate-package-lock": "node ./validate-package-lock.js", | ||
| "validate-tsconfig": "node ./validate-tsconfig.mjs", | ||
| "check-local-changes": "node ./check-local-changes.js" |
There was a problem hiding this comment.
Let us sort these alphabetically
| "check-installed-deps": "node ./check-installed-deps.mjs", | |
| "check-licenses": "node ./check-licenses.mjs", | |
| "validate-package-lock": "node ./validate-package-lock.js", | |
| "validate-tsconfig": "node ./validate-tsconfig.mjs", | |
| "check-local-changes": "node ./check-local-changes.js" | |
| "check-installed-deps": "node ./check-installed-deps.mjs", | |
| "check-licenses": "node ./check-licenses.mjs", | |
| "check-local-changes": "node ./check-local-changes.js", | |
| "validate-package-lock": "node ./validate-package-lock.js", | |
| "validate-tsconfig": "node ./validate-tsconfig.mjs" |
| 'tools/validation/**/*.js', | ||
| 'tools/validation/**/*.mjs', |
There was a problem hiding this comment.
We don't need this, it's already covered by tools/** above
| 'tools/validation/**/*.js', | |
| 'tools/validation/**/*.mjs', |
There was a problem hiding this comment.
@manzoorwanijk The linting tests are failing because the current tools/**/*.js and tools/**/*.mjs patterns don't cover .cjs files. Since there are only two cjs files in validation, should I add tools/**/*.cjs to the pattern, or would using eslint-disable-next-line no-console comments be a good idea here?
There was a problem hiding this comment.
Then we can add tools/**/*.cjs there.
manzoorwanijk
left a comment
There was a problem hiding this comment.
Thank you for making the requested changes. This looks good now.

What?
Part of #75041
Why?
The issue provides more context, but in short, this PR ensures the workspace package does not rely on root dependencies once dependency isolation is enforced.
How?
This PR converts validation tools into a new workspace
@wordpress/validation-toolswhile still relating the functionality.Testing Instructions
Testing Instructions for Keyboard
npm run lint:lockfilenpm run lint:tsconfignpm run other:check-local-changesnpm run other:check-licensesor
npm run lint