fix(build-tools): mixed internal range detection #18828
Conversation
github-actions
bot
added
area: build
| @@ -435,5 +470,6 @@ export function detectInternalVersionConstraintType(range: string): "minor" | "p | |||
| const minor = bumpInternalVersion(minVer, "minor"); | |||
|
|
|||
| const maxSatisfying = semver.maxSatisfying([patch, minor], range); | |||
| // maxSatisfying could return null in case of exact version. Should that return as "minor"? | |||
There was a problem hiding this comment.
That should probably return "exact", which would change the function signature - which is also fine.
There was a problem hiding this comment.
What should bumpRange do when it encounters and exact constraint? throw?
| // internalPrerelease only applies to versions, which won't have ~ or ^. | ||
| // internal ranges are restricted to uses of `>=` even though semver would permit more. | ||
| // For prerelease spec (internal range), ~ and ^ are interpreted as >= by semver. | ||
| // Why not just use the bump and checks all the time? | ||
| if (range.startsWith("~")) { |
There was a problem hiding this comment.
~ and ^ are checked because there are/were cases where an internal version was naively prepended with one of those, even though it's not valid technically. What should this function do if passed a value like ^2.0.0-internal.1.0.0?
There was a problem hiding this comment.
Are there cases now? Why aren't they getting fixed?
The comment I added is not really correct. But I don't think such cases should be treated as internal because the semver meaning of them is treat ~ and ^ as >=(given spec) to ^(given spec less prerelease), at least as far as I can ascertain.
I would say they should just be rejected - throw as invalid.
There was a problem hiding this comment.
After looking at the uses of this, I go back to my original comment as correct. This is not reachable except in test code. The one real use is bumpRange. But a range like ~2.0.0-internal.1.0.0 is detected as 'semver' and not internal.
To be explicit about my how does semver treat ~2.0.0-internal.1.0.0 in prior, that would become >=2.0.0-internal.1.0.0 <2.1.0.
| [true, ">=2.0.0-internal.1.0.0 <2.0.0-internal.1.1.0", undefined], | ||
| [true, ">=2.0.0-rc.1.0.0 <2.0.0-rc.1.1.0", undefined], | ||
| [false, ">=2.0.0-internal.1.0.0 <2.0.0-rc.1.1.0", undefined], |
There was a problem hiding this comment.
nit: FWIW, I find this format harder to read and understand. I think it's mostly that the test case "name" isn't easy to read. If I see "~2.0.0-internal.2.2.1 is not internal" I know what the test is checking without looking at the code.
There was a problem hiding this comment.
I think you are referring to the displayed test names shown in test output or test explorers. Such as:
✔ >=2.0.0-alpha.2.2.1 <2.0.0-alpha.3.0.0 is internal when allowAnyPrereleaseId is true
✔ >=2.0.0-dev.2.2.1.12345 <2.0.0-dev.3.0.0 is internal when allowAnyPrereleaseId is true
✔ >=1.0.0 <2.0.0 is not internal
✔ >=2.0.0-2.2.1 <2.0.0-3.0.0 is not internal
✔ ^2.0.0-internal.2.2.1 is not internal
✔ ~2.0.0-internal.2.2.1 is not internal
Those are preserved with this change. The above is from my local test run. Please let me know if you meant that or something else.
There was a problem hiding this comment.
Yeah, saw that. I was referring to the source, though. It's probably just me - I can't look at that table and parse it easily.
There was a problem hiding this comment.
I agree for the source. It isn't just you. I like the table because it addresses some issues with typos and making the file too long to see all of the cases which makes it hard to see what isn't there. There are two alternate patterns.
- Instead of array of arrays use array of records which has the parameters named and is a bit easier to follow a single line but adds verbosity to the table which makes it bigger again.
- Use a helper function and just call it a bunch of times in a row.
better inspect complex ranges with || handle cross prerelease rejection in single range set refactor tests to see examples in table form
jason-ha
force-pushed
the
build-versions-robust-internals
branch
from
9568eb3
to
44378a0
Compare
jason-ha
dismissed
tylerbutler’s stale review
Looking for formal re-review per comments and updates
|
My only concern is if test-version-utils in the client release group is expecting ^2.0.0-internal.1.0.0 to be identified as internal. Haven't had time to check myself. Everything else looks good. |
@tylerbutler, that would be a problem in test-version-utils because ^2.0.0-interna.1.0.0 is not restricted to internal versions. And that is something that the current tests check. Test case is "^2.0.0-internal.2.2.1 is not internal". There is a question for you about handling of exact constraint during bump. |
better inspect complex ranges with ||
handle cross prerelease rejection in single range set
refactor tests to see examples in table form