Repository navigation
Fix unversioned private package handling, tagging of ignored - #1361
Conversation
🦋 Changeset detectedLatest commit: bd83660 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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. |
|
Tests fail for me, but also fail on main. Not sure what I'm doing wrong. |
|
App |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1361 +/- ##
==========================================
+ Coverage 80.05% 80.23% +0.18%
==========================================
Files 53 54 +1
Lines 2201 2206 +5
Branches 647 648 +1
==========================================
+ Hits 1762 1770 +8
+ Misses 435 432 -3
Partials 4 4 ☔ View full report in Codecov by Sentry. |
2f5f69d to
2b0ac15
Compare
|
|
||
| expect(git.tag).not.toHaveBeenCalled(); | ||
| await tag(cwd); | ||
| await tag(cwd, defaultConfig); |
There was a problem hiding this comment.
i'd probably go with readConfig like in the status' tests but this is fine too. Our test suite isn't particularly consistent when it comes to this
| await tag(cwd); | ||
| await tag(cwd, { | ||
| ...defaultConfig, | ||
| privatePackages: { version: true, tag: true }, |
There was a problem hiding this comment.
hm, doesn't this change mean it's a breaking change? 🤔 note to myself: i need to recheck the current defaults
There was a problem hiding this comment.
the current default is privatePackages: { version: true, tag: false }. The current behavior was broken and this adjustment just reflects that the bug was fixed
There was a problem hiding this comment.
Hey @Andarist - Would this be the cause of my private packages no longer receiving tags with the default value of privatePackages (i.e. unset in my config)?
Some documentation for privatePackages would be a massive help - I wasn't able to find any.
There was a problem hiding this comment.
Yeah, that's effectively what this change did... I didn't realize the default option here was tag: false, but if it's tag: false, surely that means it shouldn't be tagging, right? So you should just set it to true?
There was a problem hiding this comment.
surely that means it shouldn't be tagging, right? So you should just set it to true?
Yes, I'm just asking to make sure because a default behaviour has changed on a patch version increment, on an undocumented property :)
I have tested it with tag: true, and we're back to the previous behaviour.
There was a problem hiding this comment.
Some documentation for privatePackages would be a massive help - I wasn't able to find any.
I agree... PRs are more than welcome 😉
Sorry for the breaking your workflow here. I had to recheck this myself when reviewing this PR but I concluded it was a bug. IIRC, older versions of the CLI didn't tag private packages - at some point that was broken.
There was a problem hiding this comment.
Not a problem! Was just checking I wasn't going mad. It must have been broken for a fair few months (i.e. since before we integrated changesets!)
There was a problem hiding this comment.
Yeah, it might be. The last year wasn't a particularly active year when it comes to the development of this package 😅
There was a problem hiding this comment.
Hi. This was a breaking change because:
- My repo which I don't publish (thus private) stopped tagging versions.
- The referenced
privatePackagesconfiguration is not documented, so it doesn't matter what the default was if nobody using the library knew the configuration existed.
Do I add privatePackages: { version: true, tag: true } to my .changeset/config.json to make it tag again? I see it is in the $schema.
| ); | ||
| }); | ||
|
|
||
| it("should not throw if dependents of unversioned private packages are not explicitly listed in the ignore array", async () => { |
There was a problem hiding this comment.
note to myself: recheck what this test is about and how it behaves today
There was a problem hiding this comment.
i guess what i'm surprised here is that this test has no changesets, it just tests a "dry" version run
There was a problem hiding this comment.
So it turns out that version has some validation logic in it already so this throws an error on main:
"The package "pkg-a" depends on the ignored package "pkg-b", but "pkg-a" is not being ignored. Please pass "pkg-a" to the `--ignore` flag."
The test, indeed, validates a fixed behavior 👍
This works for
versionandtag, at least in my testing for microsoft/TypeScript-Website#3116.However, there seem to be other places that should be checking this, but have less convenient access to actual Package objects. That and it's not clear to me if the other cases actually matter.Also, needs tests...And to figure out where to move this shared code as there is no common package for these besidestypeswhich is types only.