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

Compel users to release new versions of dependencies alongside their dependents #102

Merged
merged 13 commits into from
Oct 11, 2023

Conversation

cryptodev-2s
Copy link
Contributor

@cryptodev-2s cryptodev-2s commented Sep 28, 2023

This PR Compel users to release new versions of dependencies alongside their dependents

Issue: 91

@cryptodev-2s cryptodev-2s requested a review from a team as a code owner September 28, 2023 14:45
src/release-specification.test.ts Outdated Show resolved Hide resolved
src/release-specification.test.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Show resolved Hide resolved
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This is looking better! Just some user experience tweaks and attempts to simplify the logic so it's a bit easier to parse.

src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.test.ts Outdated Show resolved Hide resolved
src/release-specification.test.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
src/release-specification.test.ts Outdated Show resolved Hide resolved
tests/unit/helpers.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@legobeat legobeat left a comment

Choose a reason for hiding this comment

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

https://github.com/MetaMask/create-release-branch/actions/runs/6475554242/job/17582760139?pr=102

Error: src/package-manifest.ts(32,12): error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
Error: src/package-manifest.ts(32,39): error TS2339: Property 'Dependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(33,12): error TS1170: A computed property name in a type literal must refer to an expression whose type is a literal type or a 'unique symbol' type.
Error: src/package-manifest.ts(33,39): error TS2339: Property 'PeerDependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(88,30): error TS2339: Property 'Dependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(92,30): error TS2339: Property 'PeerDependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(306,33): error TS2694: Namespace '"/home/runner/work/create-release-branch/create-release-branch/node_modules/@metamask/action-utils/dist/package-utils".ManifestFieldNames' has no exported member 'Dependencies'.
Error: src/package-manifest.ts(307,33): error TS2694: Namespace '"/home/runner/work/create-release-branch/create-release-branch/node_modules/@metamask/action-utils/dist/package-utils".ManifestFieldNames' has no exported member 'PeerDependencies'.
Error: src/package-manifest.ts(354,31): error TS2339: Property 'Dependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(359,31): error TS2339: Property 'PeerDependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(367,32): error TS2339: Property 'Dependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/package-manifest.ts(368,32): error TS2339: Property 'PeerDependencies' does not exist on type 'typeof ManifestFieldNames'.
Error: src/release-specification.ts(218,17): error TS2339: Property 'dependencies' does not exist on type 'ValidatedPackageManifest'.
Error: src/release-specification.ts(218,31): error TS2339: Property 'peerDependencies' does not exist on type 'ValidatedPackageManifest'.
Error: src/release-specification.ts(328,36): error TS2339: Property 'dependencies' does not exist on type 'ValidatedPackageManifest'.
Error: src/release-specification.ts(329,36): error TS2339: Property 'peerDependencies' does not exist on type 'ValidatedPackageManifest'.
Error: Process completed with exit code 2.

https://github.com/MetaMask/action-utils/blob/main/src/package-utils.ts#L20

src/release-specification.test.ts Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Suggestions to fix the TypeScript errors.

src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
src/package-manifest.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

This looks great! Just two more things and then I think we can call this good:

src/package-manifest.ts Outdated Show resolved Hide resolved
src/release-specification.ts Outdated Show resolved Hide resolved
@mcmire mcmire dismissed legobeat’s stale review October 11, 2023 18:52

Linting errors have been addressed.

Copy link
Contributor

@mcmire mcmire left a comment

Choose a reason for hiding this comment

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

Sorry this took so long and thanks for sticking with it. Looks good now!

@cryptodev-2s cryptodev-2s merged commit 3556dee into main Oct 11, 2023
19 checks passed
@cryptodev-2s cryptodev-2s deleted the feature/compel-users-to-release-dependencies branch October 11, 2023 19:17
cryptodev-2s added a commit to MetaMask/core that referenced this pull request Dec 7, 2023
## Explanation

This PR updates the release process to take in considerations recent
changes on create-release-branch.
- Compel users to release packages with breaking changes alongside their
dependents [MetaMask/create-release-branch#101]
- Compel users to release new versions of dependencies alongside their
dependents [MetaMask/create-release-branch#102]
- Reorder workflow to update changelogs first
[MetaMask/create-release-branch#109]

## References

- Closes #1741

## Changelog

N/A

## Checklist

- [x] I've updated the test suite for new or updated code as appropriate
- [x] I've updated documentation (JSDoc, Markdown, etc.) for new or
updated code as appropriate
- [x] I've highlighted breaking changes using the "BREAKING" category
above as appropriate
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.

4 participants