Validation: Add a published-dependency audit script#79094
Conversation
|
@manzoorwanijk If it were distributed as a GitHub Action, would that maybe be an option to isolate it enough to avoid that dependency on Node 24 ? Or I guess even within this project, if we'd have an appetite to have some CI checks run on Node 24+ only. That means people may not be able to reliably run it locally, but this feels like a pretty rare and clear-enough output to signal from CI alone. |
|
I am instead widening the floor to add Node v20 support in the tool - manzoorwanijk/mawesome#48 I will remove the Node v20 support once Gutenberg migrates. |
9c85307 to
a6d2bff
Compare
|
Size Change: 0 B Total Size: 8.05 MB |
|
Flaky tests detected in f58fee3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/27819792931
|
a6d2bff to
14f482e
Compare
09d9195 to
92d5691
Compare
92d5691 to
4aa2bca
Compare
|
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. |
4aa2bca to
3bb460e
Compare
aduth
left a comment
There was a problem hiding this comment.
I think Windows compatibility should be confirmed, but otherwise this looks like it's in good shape 👍
| { | ||
| "comment": "equivalent-key-map ships no type declarations and has no @types/* package. Referenced only in @wordpress/data's emitted .d.ts.", | ||
| "package": "equivalent-key-map" | ||
| }, |
3bb460e to
4a7da8a
Compare
|
@aduth while working on this PR, I noticed that if I do There is also |
Oh wow, so npm doesn't validate release age if the package update is already in the lockfile? That's rather concerning, since I've been trusting the release age behavior when running |
|
Probably, the npm theory is that a poisoned dependency (inside the release age) shouldn't end up in lockfile because |
4a7da8a to
6185da5
Compare
Wire @mawesome/dependency-audit into tools/validation and expose it as `npm run lint:published-deps`. The audit verifies that every bare import reachable in each package's published artifact (runtime JS and emitted .d.ts) is declared and resolvable from the package's own dependencies, catching modules that resolve in the monorepo only via hoisting, a devDependency, or a workspace link. The ignore config documents the known-intentional findings (test fixtures, optional imports) and the remaining deferrals, each with the reason and the follow-up needed. No CI gate yet; this only adds the script so it can be run locally and evaluated. On current trunk the audit reports the 21 real findings fixed by the companion PR.
Co-authored-by: Andrew Duthie <1779930+aduth@users.noreply.github.com>
13f8093 to
f58fee3
Compare
aduth
left a comment
There was a problem hiding this comment.
LGTM 👍 It's kinda slow (1m35s), and in reviewing the internal implementation I can see why with how it's TypeScript-analyzing the packed package in an isolated context, though I can also understand why it has to be that way vs. something more naive like just looking at import sources in the published files for things like the @types/react case you're trying to explicitly catch. So I think it's a worthwhile tradeoff.
| "scripts": { | ||
| "check-installed-deps": "node ./check-installed-deps.mjs", | ||
| "check-licenses": "node ./check-licenses.mjs", | ||
| "dependency-audit": "dependency-audit --collapse-root-cause \"../../packages/*\"", |
There was a problem hiding this comment.
You'd know better than I if this will work as intended, but flagging that you document --collapse-root-cause as applying for a "a multi-target run", and it's not clear to me if the changes from glob expansion to string glob (which in effect reduces arguments from multiple to one) will produce the result we expect.
There was a problem hiding this comment.
The global here results in a multi target run - i.e. multiple packages audited together.
| "resolve.exports": "^2.0.3", | ||
| "tar": "^7.5.16", | ||
| "tinyglobby": "^0.2.17", | ||
| "typescript": "^6.0.3" |
There was a problem hiding this comment.
Should this have been a peer dependency? Especially with TypeScript 7.0 being imminent, I wouldn't want us to have to be pulling multiple major versions of TypeScript if we can avoid it, and this is a pretty heavy dependency to put on downstream consumers (no dependencies, but 24MB installed).
There was a problem hiding this comment.
No, it’s needed as a runtime dependency because the package uses the programmatic API from TypeScript which tsgo doesn’t ship yet. It will be available from TS 7.1.
| - name: Audit published dependencies | ||
| if: ${{ matrix.annotations }} | ||
| run: npm run lint:published-deps -- --fail-unused-ignores | ||
|
|
There was a problem hiding this comment.
Your PR message says "This PR only adds the script and its ignore config so the check can be run locally and evaluated; it does not add a CI gate yet", but this is a CI gate, isn't it? Is that comment outdated?
What?
Follow up to #78882 (comment), where integrating a check against undeclared published dependencies was suggested.
Adds
@mawesome/dependency-audittotools/validationand exposes it asnpm run lint:published-deps. The audit verifies that every bare import reachable in each package's published artifact — runtime JS and emitted.d.ts— is declared and resolvable from that package's own dependency ranges, never via root hoisting, adevDependency, or a workspace link.This PR adds a script and its ignore config so the check can be run locally and evaluated; it also adds a CI gate. A companion PR (#79095) fixes everything the audit currently reports — until it lands, this PR doubles as a live demonstration of what the tool catches.
Why?
Missing published dependencies have been a recurring, hard-to-catch bug class (#74310, #74655, #78882): a module resolves at build time in the monorepo, but consumers installing the package from npm cannot resolve it. For type-surface gaps the failure is silent — with
skipLibCheck: truethe affected exports degrade toany. We had no protection against regressions, and as #78882 showed, new gaps keep being introduced.On current
trunk,npm run lint:published-depsreports 21 real findings across 12 packages (all fixed by #79095), for example:With #79095 merged (and this branch rebased), the audit runs clean and deterministically:
126 packages, 0 findings, 1 collapsed, 12 ignored, 8 notices(exit 0).How?
@mawesome/dependency-auditis added as a devDependency of the private@wordpress/validation-toolsworkspace (keeping it out of the root, per workspace conventions), with adependency-auditscript that auditspackages/*.lint:published-depsscript delegates to it, mirroringlint:lockfile/lint:tsconfig.tools/validation/dependency-audit.config.jsondocuments every intentional suppression with the reason and the follow-up needed. The remaining ignores are: test-fixture imports ine2e-tests(scoped to that package'splugins/**path), the optionalpuppeteer-core/install/puppeteer-firefoximports inscripts, two libraries that ship no types and have no@types/*package (equivalent-key-map,moment-timezone's deep import), two ESM/CJS packaging mismatches to fix separately (edit-site-init→boot,interactivity-router→interactivity), and one temporary rule for@tannin/sprintfpending a tool release that resolves ESM-only types.--collapse-root-causemakes findings whose root cause is another audited package's packaging notice (currently:editorcannot resolve@wordpress/block-editor's types becauseblock-editordoes not expose them) report against the producer instead of failing every consumer.Exit codes:
0clean,1findings,2a package could not be audited — so the script is CI-ready once we decide to gate (intended follow-up, possibly after evaluating it on a few real PRs).Testing Instructions
npm cinpm run build(the audit reads the built artifacts)npm run lint:published-deps— on this branch (current trunk) expect exit 1 with21 findings, each carrying a remediation suggestion; after Packages: Fix the published dependency surface of npm packages #79095 merges and this branch rebases, expect exit 0 with126 packages, 0 findings, 1 collapsed, 12 ignored, 8 notices, 3 skipped."redux"frompackages/editor/package.jsondependencies, runnpm install && npm run lint:published-deps, and observe theundeclaredfinding reappear.Testing Instructions for Keyboard
N/A — tooling only, no UI.
Screenshots or screencast
N/A.
Use of AI Tools
Used Claude Code to wire up the tool, run the audits, and author the ignore config. The diff was reviewed before submission.