Accept tag dependencies as valid #704
Conversation
馃 Changeset detectedLatest commit: 3281faa The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 3281faa:
|
| @@ -267,6 +267,37 @@ describe("running version in a simple project", () => { | |||
| expect(changesetDir).toContain(".ignored-temporarily.md"); | |||
| }); | |||
|
|
|||
| it("should not update a dependant that uses a tag as a dependency rage for a package that could otherwise be local", async () => { | |||
There was a problem hiding this comment.
This test is already passing on the main branch but I have thought that it might be worth it to add it here nevertheless. The only difference is that this will no longer print errors in the console - this is being checked in a unit test for getDependencyGraph so I've skipped rechecking this here.
Andarist
force-pushed
the
fix/tag-dependants
branch
from
b253f72
to
3281faa
Compare
| if (!semver.satisfies(expected, depVersion)) { | ||
| const range = getValidRange(depRange); | ||
|
|
||
| if ((range && !range.test(expected)) || isProtocolRange(depRange)) { |
There was a problem hiding this comment.
Isn't isProtocolRange(depRange) doing nothing here(and also the other place it's used)? Since if it's a "protocol range" then it's not a valid semver range?
There was a problem hiding this comment.
Not quite - for instance link: would result in range === null and we might not want to accept it for production dependencies. In general - this code might not be complete, but it's more complete than the previous version. I want to understand the use cases better before allowing for more things.
Some more information can be deduced from #704 (comment)
There was a problem hiding this comment.
Oh sorry, I was reading this wrong
Somewhat, kinda - there is not an obvious way to distinguish a tag from other things. If we take a look at this list we can distinguish such things:
We know though that we also have other protocols, like So the code in this PR doesn't target tags specifically as I don't ensure that some other categories stay unmatched. However, when I look at this - those categories should probably also stay untouched in the same way. We could add some tests for those - but I'm not sure if it's worth it right now (given my limited time to work on this) |
|
Seems strange that we wouldn't error for tags and GitHub repos but would for things that have explicit protocols? Feels like we should just only error when it's a range but it's out of range of the actual version? |
We have an explicit test that disallows Like I've mentioned in other comment of mine here:
I'm unsure how all of the protocol types work with packing - and I assume that this might be different per package manager (I hope that this is not pm-dependent but I assume the worst :P). And I don't have time to investigate this right now - so I've decided to leave the existing behavior untouched and fix what I know that has to be fixed. |
No description provided.