Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

ci: enforce deduped lockfile when linting dependencies #6193

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

legobeat
Copy link
Contributor

@legobeat legobeat commented Sep 12, 2023

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

Testing instructions

Documentation

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

Breaking changes and new features

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

@legobeat legobeat marked this pull request as ready for review September 12, 2023 20:32
@haltman-at
Copy link
Contributor

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
Copy link
Contributor Author

legobeat commented Sep 12, 2023

@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 [email protected])

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

Now we have duplicates of [email protected] and [email protected]. 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.

Copy link
Contributor

@haltman-at haltman-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, seems good to me, I guess. We'll see what others think!

@legobeat
Copy link
Contributor Author

legobeat commented Sep 12, 2023

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 d37ed52 as easy as that without throwing out baby with water or getting conflicts in package.json ^^

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants