-
Notifications
You must be signed in to change notification settings - Fork 569
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
Fixed an issue with incorrect dependent updates on none changesets with updateInternalDependents: "always"
#949
Conversation
Test that when `updateInternalDependents:'always'` is set then transitive dependencies of devDependencies should not be part of the release plan.
🦋 Changeset detectedLatest commit: f344afb The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit f344afb:
|
expect(getPkgJSON("pkg-c", spy.mock.calls)).toEqual( | ||
expect.objectContaining({ | ||
name: "pkg-c", | ||
version: "1.0.0", | ||
dependencies: { | ||
"pkg-b": "1.0.0", | ||
}, | ||
}) | ||
); |
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.
Correct me if I'm wrong but this one should not be part of the valid expectation, right? In this situation the pkg-c
should be left untouched
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.
Yes, you're correct - pkg-c's package.json should be untouched. I didn't spot that getPkgJSON behaves in the same way as getChangelog
and throws an error if it wasn't touched.
I've pushed out a fix for this here - still need to recheck some other things though before I can land this. |
updateInternalDependents: "always"
Fixes and extra tests all look good to me. Thank you for taking this over @Andarist! |
This PR adds a pair of failing tests, showcasing the issue described in #947.
When
updateInternalDependents:'always'
is set then transitive dependencies of devDependencies should not be part of the release plan, howevever they currently are.I believe the root cause of this issue is somewhere around this logic. There needs to be some way of stripping out dependencies of devDependencies from the release plan. I've tried and failed to work out how to do that with small adjustments, but I hope these tests can serve as a useful starting point for an actual fix.
UPDATE: fixes #947