fix(doctor, watchman): accept new date-based watchman versions #1550
Conversation
this verifies existing functionality prior to changes
doctor was giving false-negatives with the new watchman versions they use date-based versions now that violate semver with leading-zeros in numeric parts: - npm/node-semver#232 loose mode correctly pasrses them though, so add optional parameter to install check that allows loose version range coercion Should be compatible as watchman project is on record as being permanently backwards-compatible: https://facebook.github.io/watchman/docs/compatibility.html
mikehardy
requested review from
grabbou,
thymikee and
Esemesek
as code owners
mikehardy
changed the title
|
This was needed so that |
| @@ -19,11 +19,13 @@ const isSoftwareNotInstalled = async (command: string): Promise<boolean> => { | |||
| const doesSoftwareNeedToBeFixed = ({ | |||
| version, | |||
| versionRange, | |||
| looseRange = false, | |||
There was a problem hiding this comment.
default value so no other code has to change, and follows the semver package historical precedent of all semver APIs taking loose as a final argument, with false being the default
| @@ -3,7 +3,7 @@ export default { | |||
| NODE_JS: '>= 8.3', | |||
| YARN: '>= 1.10.x', | |||
| NPM: '>= 4.x', | |||
| WATCHMAN: '4.x', | |||
| WATCHMAN: '>= 4.x', | |||
There was a problem hiding this comment.
4.9.0 was released something like 2 years ago then they transitioned to date ranges that have leading zeros in the semver parts, which is technically a semver violation but still easily parsed. This let's the range slip up to the new versions, which do work
| }): boolean => { | ||
| const coercedVersion = semver.coerce(version); | ||
| const coercedVersion = semver.coerce(version, {loose: looseRange}); |
There was a problem hiding this comment.
Does the check for version ranges just work as-is when comparing semver vs date version?
There was a problem hiding this comment.
I'm not sure I fully understand the question so let me state what I think I understand, then please correct if I'm wrong - I think that if you compare a semver vs a "semver except it has leading zeros in some parts because it is YYYY.MM.DD.ZZ.etc" it will coerce and compare and work as is. The coercion takes the leading three parts and the loose allows leading zeros to still go through and be treated as integers during coercion
Which is...sort of some detail and not an answer. I was worried personally about the impact here which is why I went with an optional parameter you can opt-in too when you know a version series "style" (that is: it has changed to date, and it has changed in a way that it's previous semver-compatible will work with it's current date form) and thus can make more assumptions / tests about correct behavior vs hoping that the full loose backus-naur form works well with the semver BNF
Hopefully that's what you were curious about?
|
Well, I guess it sort of fixes that issue. Alternative would be to enable looseMode for all the version checks. I think at this point, since we have support for CalVer, we are good to close that issue and just keep support in mind when other dependencies switch to it in the future. What you think? |
|
@mikehardy great work and thanks for adding tests. My question is - where I can look up details on what exactly is loose mode doing that makes this code work? |
It appears undocumented, I had to read the regexes in source 😂 |
Summary:
fix(doctor, watchman): accept new date-based watchman versions
doctor was giving false-negatives with the new watchman versions
they use date-based versions now that violate semver with leading-zeros in numeric parts:
loose mode correctly pasrses them though, so add optional parameter to install check that
allows loose version range coercion
Should be compatible as watchman project is on record as being permanently backwards-compatible:
https://facebook.github.io/watchman/docs/compatibility.html
Test Plan:
I wrote a test that covers watchman completely before modifying it, then modified it and added a chunk of test to check the modification - so, best reviewed one commit at a time