Skip to content

Conversation

@mcmire
Copy link
Contributor

@mcmire mcmire commented Jul 22, 2024

Explanation

The existing Yarn constraints have been difficult to maintain because they are written in Prolog. More recent versions of Yarn support JavaScript-based constraints, so this commit rewrites the existing constraints in JavaScript.

Instead of constraints.pro, all constraints are now kept in yarn.config.cjs. This file can be used for other things in the future besides constraints. (And constraints can be used in the future to check more than just dependencies!)

Note that we have had discussions in the past around the peer dependency constraints, and we know that they are not quite correct. This will be fixed in another PR.

References

Fixes #4384.

Changelog

(No consumer-facing changes.)

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

@mcmire mcmire requested a review from a team July 22, 2024 23:40
@mcmire mcmire added the team-wallet-framework Deprecated: Please use `team-core-platform` instead. label Jul 22, 2024
@mcmire mcmire requested a review from a team July 22, 2024 23:40
@socket-security
Copy link

socket-security bot commented Jul 22, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
npm/@yarnpkg/types@4.0.0 None 0 8.8 kB yarnbot

🚮 Removed packages: npm/semver@6.3.1

View full report↗︎

The existing Yarn constraints have been difficult to maintain because
they are written in Prolog. More recent versions of Yarn support
JavaScript-based constraints, so this commit rewrites the existing
constraints in JavaScript.

Instead of `constraints.pro`, all constraints are now kept in
`yarn.config.cjs`. This file can be used for other things in the future
besides constraints. (And constraints can be used in the future to check
more than just dependencies!)

Note that we have had discussions in the past around the peer dependency
constraints, and we know that they are not quite correct. This will be
fixed in another PR.
@mcmire mcmire force-pushed the convert-to-javascript-constraints branch from 49f4cf0 to aa6fa92 Compare July 23, 2024 04:34
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Make sure to review yarn.config.cjs :) It is being hidden in the diff.

@cryptodev-2s
Copy link
Contributor

Looks good to me. However, there are two rules that are missing which it will be nice if we can add them. Additionally, it would be helpful to create a ticket to address the inconsistency with some rules. For instance, we check that changelog:validate has the correct value, but if it doesn't exist, no error is thrown. Same for changelog:update and others...

@cryptodev-2s
Copy link
Contributor

We should add a summary of the rules we are checking at the top of the file, like this:

/**
 * Yarn Workspace Constraints Configuration
 *
 * This configuration ensures that all workspaces in the monorepo adhere to a consistent set of rules and conventions.
 *
 * Rules:
 * 
 * 1. **Workspace Name**:
 *    - All packages must have a name.
 *    - Non-root packages must have a name that matches their directory (`@metamask/{dirname}`).
 *
 * 2. **Package Version**:
 *    - All non-root packages must have a version.
 *
 * 3. **Package Description**:
 *    - All packages must have a description that does not end with a period.
 *
 * 4. **NPM Keywords**:
 *    - Non-root packages must have the keywords `MetaMask` and `Ethereum`.
 *
 * 5. **Homepage URL**:
 *    - Non-root packages must have a homepage URL that includes their name and points to the correct repository.
 *
 * 6. **Bugs URL**:
 *    - Non-root packages must have a bugs URL pointing to the Issues page of the repository.
 *
 * 7. **Repository**:
 *    - All packages must specify Git as the repository type.
 *    - The repository URL must point to a MetaMask GitHub repository.
 *
 * 8. **License**:
 *    - Default license for non-root packages is MIT, except for specific packages (`ISC` or custom).
 *
 * 9. **Side Effects**:
 *    - Non-root packages must not have side effects, except `@metamask/base-controller`.
 *
 * 10. **Exports**:
 *     - Non-root packages must set up ESM- and CommonJS-compatible exports correctly.
 *
 * 11. **Build and Test Scripts**:
 *     - Non-root packages must have the same `build`, `build:docs`, `test`, `test:clean`, `test:verbose`, and `test:watch` scripts.
 *     - Non-root packages must not have a `prepack` script.
 *     - Non-root packages must have a valid `changelog:validate` script.
 *     - Non-root packages must have a valid `changelog:update` script.
 *
 * 12. **Included Files**:
 *     - Non-root packages must only include files generated during the build process.
 *     - Root package must specify an empty set of published files.
 *
 * 13. **Yarn Version**:
 *     - The root workspace must specify the Yarn version required for development.
 *
 * 14. **Node.js Version**:
 *     - All packages must specify a minimum Node.js version of 18.18.
 *
 * 15. **Publishing**:
 *     - Non-root public packages should be published to the NPM registry.
 *     - Non-root private packages should not be published.
 *
 * 16. **README.md**:
 *     - Non-root packages must have a valid README.md file.
 *
 * 17. **Dependency Constraints**:
 *     - Dependencies and devDependencies must match the current version of the dependency.
 *     - No dependency should be listed under both dependencies and devDependencies.
 *     - Controller packages must also be listed in peerDependencies.
 *     - Version ranges for the same dependency must be consistent across the monorepo.
 *
 * This configuration is enforced by defining rules and validations within the Yarn constraints.
 */

@mcmire
Copy link
Contributor Author

mcmire commented Jul 25, 2024

@cryptodev-2s We could add a summary at the top, but I'm already summarizing each step, and then on top of that I have documentation for each function that explains how certain constraints work. So if we added a summary, that would be a third form of documentation that people would have to remember to update if they updated the constraints. I guess we can regenerate it easily using ChatGPT, but maybe there's another way we can do this? I am sensing that if we need this, the constraints function is too long and difficult to read. Is that your view? If I extracted every constraint to its own function, would that make it easier to scan?

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!

@mcmire mcmire merged commit 206e81b into main Jul 26, 2024
@mcmire mcmire deleted the convert-to-javascript-constraints branch July 26, 2024 17:14
AugmentedMode pushed a commit that referenced this pull request Jul 30, 2024
The existing Yarn constraints have been difficult to maintain because
they are written in Prolog. More recent versions of Yarn support
JavaScript-based constraints, so this commit rewrites the existing
constraints in JavaScript.

Instead of `constraints.pro`, all constraints are now kept in
`yarn.config.cjs`. This file can be used for other things in the future
besides constraints. (And constraints can be used in the future to check
more than just dependencies!)

Note that we have had discussions in the past around the peer dependency
constraints, and we know that they are not quite correct. This will be
fixed in another PR.

---------

Co-authored-by: cryptodev-2s <109512101+cryptodev-2s@users.noreply.github.com>
sirtimid added a commit to MetaMask/ocap-kernel that referenced this pull request Sep 6, 2024
## Explanation

Migrated constrains. [This
PR](MetaMask/core#4546) was used as a base with
various customizations to align with our constraints.

See comments for adjustments to the prolog constraints.

## References

Fixes [#17](#17)

---------

Co-authored-by: Dimitrios Marlagkoutsos <dimitrismarl@opg7h1k69.home>
Co-authored-by: Erik Marks <25517051+rekmarks@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

team-wallet-framework Deprecated: Please use `team-core-platform` instead.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

The constraints file is difficult to maintain

3 participants