-
-
Notifications
You must be signed in to change notification settings - Fork 5
Detect direct dependents missing from release #193
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
Conversation
| /> | ||
| )} | ||
| {packageDependencyErrors[pkg.name].missingDependentNames.length > | ||
| {packageDependencyErrors[pkg.name].missingDirectDependentNames |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note there aren't any tests for these changes. I think we'd need to do some refactoring to this component to make that easy, as this component has a lot of responsibilities. I didn't want to get into this right now, but maybe in a future PR.
| * @returns An array of package names. | ||
| */ | ||
| export function findAllWorkspacePackagesThatDependOnPackage( | ||
| export function findWorkspaceDependentNamesOfType( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note there aren't any tests for the functions we're updating here. We probably should do that eventually, but in the meantime they're tested implicitly via release-specification.test.ts.
If the user includes a package in the release and selects a new major version, currently, we display a message that requires the user to include all peer dependents in the release. However, since we moved peer dependencies to dependencies in the `core`, realistically, this error will never appear, and instead we want to strongly advise the user to include all *direct* dependents in the release. To do this we have to modify the core logic to detect missing direct dependents separately from peer dependents.
af7baeb to
628bf64
Compare
src/release-specification.test.ts
Outdated
| }); | ||
| }); | ||
|
|
||
| // NEW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug comment accidentally left in test file
Low Severity
A // NEW comment appears to be a development artifact that was left behind when adding the new direct dependents test cases. This comment doesn't serve any documentation purpose in the final code and appears to have been used during development to mark newly added tests.
| }); | ||
| }); | ||
|
|
||
| it('throws if there are any packages in the release with a major version bump using the word "major", but any of their direct dependents have changes since their latest release and are not listed in the release', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are way too many tests here, and I need to refactor this. A problem for another day...
cryptodev-2s
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
If the user includes a package in the release and selects a new major version, currently, we display a message that requires the user to include all peer dependents in the release. However, since we moved peer dependencies to dependencies in the
corerepo, realistically, this error will never appear, and instead we want to strongly advise the user to include all direct dependents in the release.To do this we have to modify the core logic to detect missing direct dependents separately from peer dependents.
References
https://consensyssoftware.atlassian.net/browse/WPC-330
Screenshots
Before
After
Notice the new "Missing Direct Dependents" section:
Note
Introduces explicit handling of direct dependents when a package is major-bumped, alongside existing peer dependent checks.
findWorkspaceDependentNamesOfType,findCandidateDependentsOfTypeForMajorBump,findCandidateDependencies) and updates validation to emit a new error for missingdirectdependents with tailored guidance; retains peer-dependent errors with minor copy tweaks/api/packagesnow includes required direct and peer dependents;/api/check-packagesreturnsmissingDirectDependentNamesandmissingPeerDependentNames; response types adjustedApp.tsxandPackageItem.tsxWritten by Cursor Bugbot for commit 9e36c12. This will update automatically on new commits. Configure here.