ci: enforce deduped lockfile when linting dependencies

legobeat commented 10 months ago

PR description

New workspace package scripts
- deduplicate:check: exits with error if any redundancies in yarn.lock are detected
- deduplicate: performs a best-effort attempt to resolve such duplicates
- lint:dependencies: deduplicate:check && depcheck
- lint:dependencies:fix: deduplicate && depcheck
ci: fail if yarn lint:dependencies errors
new devDependency yarn-deduplicate
- see this package for details about how the dedupe is done and possible configuration (the main one being if highest or fewer should be used - this uses fewer but it's a tradeoff and up to maintainer to decide if fewer or highest is preferred)

Blocked by

6194
- though the scripts could be broken out separately without the ci checks to make those not dependent, if this is desired

Testing instructions

Documentation

[x] I thought about documentation and added the doc-change-required label to this PR if documentation updates are required.

Breaking changes and new features

[x] I have added any needed breaking-change and new-feature labels for the appropriate packages.

haltman-at commented 9 months ago

Nice! The one thing I'm wondering about here is -- is there any possibility that yarn-deduplicate can fail, in such a way that the test would not pass without manual intervention? You describe it above as "best-effort". I'm wondering that means it can get stuck in a fail state like that.

legobeat commented 9 months ago

@haltman-at I don't see how that would happen but I also can't promise it won't (: Thinking if it's just throwing a fit or being inconsistent, --exclude can be used to ignore certain packages (or the entire yarn-deduplicate step could be removed or reduced to a warning)

The one thing I can think of that could require intervention, though is a hypothetical future scenario like:

# Before change
truffle > foo > bar@^1.1.0 (resolved to bar@1.1.0)

# After change that adds new dependency `zed`
truffle > foo > bar@^1.1.0 (resolved to bar@1.1.0)
truffle > zed > bar@^1.1.1 (resolved to bar@1.1.1)

Now we have duplicates of bar@1.1.0 and bar@1.1.1. Now let's say that bar doesn't actually respect semver and there were breaking changes in both directions between the two versions so dedulication shouldn't actually be done (well, maybe another answer here is, why are we adding new dependencies on this messy package/why aren't the intermediary dependencies pinning it? ;))

If this were to happen and resolving it through upstreams isn't feasible or sufficient, I would reach for resolutions which would also capture this pinning inside package.json

(For devDependencies. Relying on resolutions in yarn.lock is I guess OK for devDeps like eslint and lerna but unacceptable for runtime dependencies as they aren't followed when a user installs the package from registry. Unfortunately semantics around resolutions differs between package managers so I wouldn't rely on it for non-bundled runtime dependencies in public packages)

Bottom line I guess: If transitive dependencies violate semver then unexpected things may happen regardless - but bringing those issues to light in CI sooner than later is probably preferrable.

legobeat commented 9 months ago

could also break out the commits into 2 separate PRs (or do a rebase-merge, in case you practice those on this repo), which would make reverting d37ed52e5ea3bdff951c7be7736f1e47c483e8eb as easy as that without throwing out baby with water or getting conflicts in package.json ^^

trufflesuite / truffle