Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dependency-related constraints #5464

Merged
merged 4 commits into from
Mar 14, 2025

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Mar 12, 2025

Explanation

I've noticed recently that it is impossible to add a controller dependency just to dependencies; one of our existing constraints demands that it also be added to peerDependencies. This is obviously incorrect.

This commit addresses this (and other related constraints) by the following:

  • Remove expectControllerDependenciesListedAsPeerDependencies. This is the faulty constraint referred to above.
  • Correct expectDependenciesNotInBothProdAndDev (now expectDependenciesNotInBothProdAndDevOrPeer) to not only ensure that a dependency is not listed in both dependencies and devDependencies but also both dependencies and peerDependencies (only devDependencies + peerDependencies is allowed, as combinations go).
  • Add expectPeerDependenciesAlsoListedAsDevDependencies constraint to ensure that peer dependencies are also listed as dev dependencies.
  • Fix expectUpToDateWorkspaceDependenciesAndDevDependencies to skip validation of "production" dependencies that are also listed in peer dependencies (this avoids conflicting constraints).
  • Correct expectConsistentDependenciesAndDevDependencies to skip workspace dependencies, as that is already being handled by expectUpToDateWorkspaceDependenciesAndDevDependencies (this avoids conflicting constraints).

Note that because of these changes, several controllers have also been
corrected to fit.

References

(N/A)

Manual testing steps

  • It should be possible to add a controller package to dependencies without needing to add it to peerDependencies. To test this:
    • Add "@metamask/permission-controller": "^11.0.6", to dependencies in the manifest for @metamask/accounts-controller
    • Run yarn constraints
    • You should not see any errors.
  • It should not be possible to add a package to both dependencies and peerDependencies. To test this:
    • Add "@metamask/permission-controller": "^11.0.6", to dependencies and peerDependencies in the manifest for @metamask/accounts-controller
    • Run yarn constraints
    • You should see an error.
  • It should not be possible to add a package to just peerDependencies (it must also be present in devDependencies). To test this:
    • Add "@metamask/permission-controller": "^11.0.6", to just peerDependencies in the manifest for @metamask/accounts-controller
    • Run yarn constraints
    • You should see an error asking to also add the package to dependencies.
    • Change the line to "@metamask/controller-utils": "^11.6.0",
    • Run yarn constraints
    • You should still see an error asking to also add the package to dependencies.

Changelog

(Updated in PR.)

Checklist

  • I've updated the test suite for new or updated code as appropriate
  • I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate
  • I've highlighted breaking changes using the "BREAKING" category above as appropriate
  • I've prepared draft pull requests for clients and consumer packages to resolve any breaking changes

@mcmire mcmire force-pushed the allow-controller-to-only-be-listed-in-deps branch 3 times, most recently from cc436b1 to 8517743 Compare March 12, 2025 21:10
@mcmire mcmire marked this pull request as ready for review March 12, 2025 21:18
@mcmire mcmire requested review from a team as code owners March 12, 2025 21:18
This commit adjusts the constraints in `yarn.config.cjs`:

- Remove `expectControllerDependenciesListedAsPeerDependencies`. As its
  name implies, this constraint ensured that dependencies which referred
  to controllers were always listed as peer dependencies. This is
  incorrect, because in some cases we may simply want to list them as
  "production" dependencies.
- Correct `expectDependenciesNotInBothProdAndDev` (now
  `expectDependenciesNotInBothProdAndDevOrPeer`) to not only ensure that
  a dependency is not listed in both `dependencies` and
  `devDependencies` but also both `dependencies` and `peerDependencies`
  (only `devDependencies` + `peerDependencies` is allowed, as
  combinations go).
- Add `expectPeerDependenciesAlsoListedAsDevDependencies` constraint
  to ensure that peer dependencies are also listed as dev dependencies.
- Fix `expectUpToDateWorkspaceDependenciesAndDevDependencies` to skip
  validation of "production" dependencies that are also listed in peer
  dependencies (this avoids conflicting constraints).
- Correct `expectConsistentDependenciesAndDevDependencies` to skip
  workspace dependencies, as that is already being handled by
  `expectUpToDateWorkspaceDependenciesAndDevDependencies` (this avoids
  conflicting constraints).

Note that because of these changes, several controllers have also been
corrected to fit.
@mcmire mcmire force-pushed the allow-controller-to-only-be-listed-in-deps branch from 8517743 to 625caf9 Compare March 12, 2025 21:19
* Expect that the workspace has the given field, and that it is a non-null
* value. If the field is not present, or is null, this will log an error, and
* cause the constraint to fail.
* This function does one of three things depending on the arguments given:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not related to this PR, but it had been a while since I'd worked on this file, and this description was confusing.

cryptodev-2s
cryptodev-2s previously approved these changes Mar 14, 2025
Copy link
Contributor

@cryptodev-2s cryptodev-2s left a comment

Choose a reason for hiding this comment

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

LGTM!

ccharly
ccharly previously approved these changes Mar 14, 2025
Copy link
Contributor

@ccharly ccharly left a comment

Choose a reason for hiding this comment

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

LGTM for accounts-related files 👍

@mcmire mcmire enabled auto-merge (squash) March 14, 2025 17:20
infiniteflower
infiniteflower previously approved these changes Mar 14, 2025
@infiniteflower
Copy link
Contributor

Looks good for Swaps/Bridge

mirceanis
mirceanis previously approved these changes Mar 14, 2025
@mcmire mcmire merged commit 2b8c61d into main Mar 14, 2025
181 checks passed
@mcmire mcmire deleted the allow-controller-to-only-be-listed-in-deps branch March 14, 2025 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants