feat/package/ranges #139

ghost · 2017-03-20T23:22:28Z

No description provided.

hutson · 2017-03-21T03:12:36Z

lib/helpers/package-json.js

@@ -30,10 +30,10 @@ function setNewValue(currentFileContent, depType, depName, newVersion) {
    return currentFileContent;
  }
  // Update the file = this is what we want
-  parsedContents[depType][depName] = newVersion;
+  parsedContents[depType][depName] = (upgradeType === 'major' ? '^' : upgradeType === 'minor' ? '~' : '') + newVersion;


There's a major flaw in my approach, as I can't reasonable decide which range character to use based on the upgradeType.

Even a pinned version may require a major upgrade.

Therefore I'll need to extract the original range character and then prepend the new version with that range.

rarkins · 2017-03-21T04:39:28Z

@destroyerofbuilds can you give me some examples of how you intend to use this?

e.g. if package.json says ^1.0.0 and a new version 1.1.0 is released, then what?
if package.json says ^1.0.0 and 2.0.0 is released, then what?
if package.json says ~1.0.0 and 1.1.0 is released, then what?

And I assume you want to also use it in conjunction with yarn features?

ghost · 2017-03-21T22:21:52Z

@rarkins using your examples, and assuming we're only interested in upgrading dependencies (this does not take into consideration how your yarn maintenance feature plays into it):

^1.0.0 is in package.json and 1.1.0 has been released.
- Nothing happens as 1.1.0 satisfies ^1.0.0.
^1.0.0 is in package.json and 2.0.0 has been released.
- Then package.json is updated to replace ^1.0.0 with ^2.0.0.
~1.0.0 is in package.json and 1.1.0 has been released.
- Then package.json is updated to replace ~1.0.0 with ~1.1.0.

A few others:

* is in package.json and 1.1.0 has been released.
- Nothing happens as 1.1.0 satisfies *.
1.0.0 is in package.json and 1.0.1 has been released.
- Then package.json is updated to replace 1.0.0 with 1.0.1.

ghost · 2017-03-21T23:00:15Z

On further thought, I don't know if it's possible to support any of those range patterns (or even all the ranges allowed per the Range Grammer).

Just as an arbitrary example, what would I do in the case of ~2.0.0 || ^1.0.0? Would I use ~ or ^?

I think at the point that the range spec is more complicated than a single ^ or ~, we can't make any assumption about what the user would like.

ghost · 2017-03-21T23:09:21Z

Perhaps the easiest would be an option for setting the range spec with the default being an empty string (which is essentially the existing behavior of renovate).

Then I could call renovate with --range="^".

rarkins · 2017-03-22T07:08:55Z

I expect that this concept is only useful to library authors, not webapp users. For a webapp users, I still can't see any reason why you wouldn't want to pin versions in package.json (+ use yarn), if using renovate. This provides more granular control than using ranges with yarn, as you might experience multiple dependency updates at once in yarn.lock, one of which is faulty.

For library authors - why use ranges instead of pinning, if also using renovate? This would be for the "commonly accepted" reason - the wider your dependency semver ranges, the lower the chance you force users to use multiple versions of the same (sub-)dependency. So this implies that "the wider the range the better - so long as they actually work".

The majority of library authors would use the default ^ minor range, while some would lock down further to use ~ patch ranges. Presumably the latter have a concern about compatibility with older minor versions but a confidence in the stability of patch ranges. Some libraries may have more advanced rules, like ~2.0.0 || ^1.0.0. In this latter case, I think it's hard for us to know what they'd want, if release 2.1.0 came out (for example).

Patch ranges (~) are probably the easiest. In this case, it seems pretty obvious that the next step would be to bump to the next minor level and maintain the patch range. e.g. if they are using ~1.1.0 and version 1.2.0 is released, then the update would be ~1.2.0. If version 1.3.0 was also available then we would be updating them to ~1.3.0 in package.json. If version 1.3.0 and 2.0.0 were both available, I think we would want to split that into separate branches like we do with pinning, e.g. one PR for ~1.3.0 and another for ~2.0.0.

Minor ranges (^1.1.0) would remain untouched until a new major is available (e.g. 2.0.0). In this case, I think it makes sense for us to raise a PR to update this dependency to ^2.0.0. If those tests pass and the user decides to update it with a more advanced semver (e.g. ^1.1.0 || ^2.0.0) then we are still helping with that.

Then finally we have the advanced semver ranges such as mentioned above. We wouldn't need to do anything until a version greater than the existing range(s) is released, e.g. 3.0.0. A few possibilities here:

We do nothing.. no support for advanced ranges
We do the simplest update possible - ^3.0.0 and let the user decide
We append the new range to the existing, e.g. ^1.1.0 || ^2.0.0 || ^3.0.0
- I'm sure there will be many edge cases here that are difficult for us to think of ahead of time

rarkins · 2017-03-22T07:15:06Z

What are our guiding principles then, if users are to enable some type of "pinVersions = false" option in renovate?

Don't convert package.json ranges to pinned versions
Do not narrow or widen semvers ourselves - if a dependency is defined by a simple range (~ or ^) then keep using that type of range.
Raise a PR if a new version of a dependency is released that doesn't satisfy their existing ranges

If using a simple ~ or ^ range already then update to the latest range
Raise multiple PRs for minor/major
If using a complex range (e.g. includes ||) then append to it. But this could get very complex. e.g. if their range is ^1.1.0 || ~2.0.0 and versions 2.1.0 and 2.2.0 are released, then what?

I'm still wondering if for complex ranges our goal may not be to automatically "guess" what new range they'll want, but instead simply to alert them to the existing of new version(s) and let them decide the new range, if our PR with new version passes their tests.

ghost · 2017-03-22T16:07:32Z

I expect that this concept is only useful to library authors, not webapp users. For a webapp users, I still can't see any reason why you wouldn't want to pin versions in package.json

From my perspective, I don't see any value in pinning dependencies in applications. To remove the risk of transitivie dependencies introducing breaking changes you must use a lockfile in your application. Once you've introduced a lockfile, you've negated any benefit of pinning dependencies in your application's package.json file. Without any benefits, using the same strategy for libraries and applications would mean less code in renovate.

as you might experience multiple dependency updates at once in yarn.lock, one of which is faulty.

I'm not sure I follow.

Are you saying that with pinned dependencies in your application's package.json file, updating a single pinned dependency with renovate, would lead to a smaller diff after running yarn upgrade (perhaps with only the changed pinned dependency changing in yarn.lock)?

How would transitive dependencies not change in yarn.lock after a yarn upgrade?

For library authors - why use ranges instead of pinning, if also using renovate?

I think you covered it in the rest of that paragraph. (A full example is here) Your third paragraph here also captures my opinion on the matter.

Presumably the latter have a concern about compatibility with older minor versions

Actually, I think it's a concern with compatibly with newer minor versions. Your comments elsewhere about Angular 1.x being a good example of a library that does not follow semver.

What are our guiding principles then, if users are to enable some type of "pinVersions = false" option in renovate?

Don't modify existing version ranges within package.json.
For simple semver ranges (pinned, ~, or ^), do not narrow or widen the semantic version range ourselves, but instead, respect the range width chosen by the project's owner.
For simple semver ranges, raise one or more pull/merge requests if a new version of a dependency is released that does not satisfy the existing range.
3.1) Pinned - Version 2.0.0/1.1.0/1.0.1 of 1.0.0 dependency is released - Raise requests for 2.0.0 and 1.1.0.
3.2) Tilde - Version 2.0.0/1.1.0/1.0.1 of ~1.0.0 dependency is released - Raise requests for ~2.0.0 and ~1.1.0.
3.3) Carrot - Version 2.0.0/1.1.0/1.0.1 of ^1.0.0 dependency is released - Raise request for ^2.0.0 and ^1.1.0.
For complex semver ranges (detected using something like semver-utils, I don't know. What would be least surprising?

codecov-io · 2017-03-23T14:33:42Z

Codecov Report

Merging #139 into master will increase coverage by 1.6%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #139     +/-   ##
=========================================
+ Coverage   68.94%   70.55%   +1.6%     
=========================================
  Files          19       19             
  Lines         805      849     +44     
  Branches      130      147     +17     
=========================================
+ Hits          555      599     +44     
  Misses        250      250

Impacted Files	Coverage Δ
lib/workers/branch.js	`100% <ø> (ø)`	⬆️
lib/config/definitions.js	`100% <ø> (ø)`	⬆️
lib/helpers/versions.js	`100% <100%> (ø)`	⬆️
lib/worker.js	`70.83% <100%> (+1.26%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e4d4e2...bc39699. Read the comment docs.

ghost · 2017-03-23T22:22:31Z

I'm slowly working my way through updating the tests to reflect the new support for range semantics.

rarkins · 2017-03-24T06:03:48Z

as you might experience multiple dependency updates at once in yarn.lock, one of which is faulty.

I'm not sure I follow.

Let's say that your direct dependencies are packages a,b,c.
Let's say you don't pin - you use semver ranges - but you do use a yarn.lock file.
So for this example, you depend on semver ^1.0.0 for all three dependencies, and the latest for each is version 1.1.0 currently. Yarn will therefore pin each of those to version 1.1.0.
Now let's say that dependency a releases version 1.2.0 at about the same time that b releases 1.1.1.
Any users of the repository can run yarn install and still get the pinned versions 1.1.0.
But as the administrator, how can you perform the update and test the new versions?
The only way is yarn upgrade, but that will upgrade both a and b to versions 1.2.0 and 1.1.1 respectively. And what if a version 1.2.0 is actually faulty and breaks your build, while b version 1.1.1 contains a must-have fix? Your only solution would be to (a) figure out which of the dependency upgrades caused the break (in reality you may have a lost more than just one dependency upgrade every day, if you are doing this process daily), then (b) work out the "last known working" version of it (manually inspect your existing yarn.lock?) and then (c) pin that back to the last known working version in your package.json.

Now compare this to the scenario where you pin versions in package.json instead:

Renovate will raise two separate PRs, one for a:1.2.0 and one for b:1.1.1
A will fail tests and B will pass
You merge B's PR
Your close the A PR and wait for renovate to raise a new one for when 1.2.1 is hopefully released soon

rarkins · 2017-03-24T06:08:14Z

It's also quite possible in webapps that a "broken" version 1.2.0 of a would not actually be detected during your own tests, and would make it to production. In both scenarios described above (pinned and unpinned dependencies), you would discover this through perhaps elevated browser exceptions or some other user report.

You could work out a way to reproduce it - or better yet add a test to detect it, but then what? In the semver range scenario, you're left to hunt back through your yarn.lock updates and try to guess which of them it was that caused it, then perform manual pinning to test.

In the pinned package.json scenario, you can simply keep rolling back each of your renovate upgrades until you get the test to pass. Then you can revert the bad renovate commit and continue.

@destroyerofbuilds this is why I'm thoroughly convinced that all webapp users should be pinning versions in package.json + using yarn & renovate.

ghost · 2017-03-24T15:48:28Z

Let's say that your direct dependencies are packages a,b,c.

I like the scenario you've setup, and I think it's valid.

What I feel you're missing are dependencies d,e,f,g,h,i,j,k,l,m,n,o,p that your dependencies a,b,c depend on in some combination, but are listed in a package.json using ^, because dependencies a,b,c haven't heard of renovate, so don't pin their own dependencies.

Therefore, when d releases a broken version, no matter how many times you roll back the pinned dependency a in your package.json file, you will continue to get the broken version of d, because d isn't pinned in any release of a, nor is it pinned in your package.json file.

So pinning dependency a did not help you to fix you broken application.

Instead you're still going to have to work through the changes in yarn.lock and manually pin packages until your build works again.

But as the administrator, how can you perform the update and test the new versions?

Both npm and yarn lack sufficient support in this area.

For direct dependencies, just run yarn upgrade [DIRECT DEPENDENCY].

I really wish they had a way to interactively upgrade transient dependencies, one at a time, for the purpose of testing compatibility.

Furthermore, I then wish they had an interactive flattening command, because once everything is upgraded, there may be opportunities to flatten the directory structure.

ghost · 2017-03-24T15:50:17Z

lib/helpers/versions.js

+
+    // If no operator exists then return an empty string.
+    // This will cause the dependency version to get pinned
+    // to a specific version, which is the existing behavior of `renovate`


@rarkins in the case where the version is either pinned, or using advance semantic range semantics, then we keep renovate's existing behavior.

Only when the semantic version range is simple do we keep the range.

Therefore applications can, if they choose, pin dependencies in their package.json file and this change will respect that.

ghost · 2017-03-24T16:31:30Z

@rarkins you may be interested in yarnpkg/yarn#2543.

They are actively working on an RFC for the feature - yarnpkg/rfcs#54

rarkins · 2017-03-25T10:45:31Z

@destroyerofbuilds by the way, I haven't really looked at any of your code here yet, as I'm still trying to imagine what the requirements are. Let me know if/when you want me to look into implementation details here.

rarkins · 2017-03-25T13:03:22Z

What I feel you're missing are dependencies d,e,f,g,h,i,j,k,l,m,n,o,p that your dependencies a,b,c depend on in some combination

I agree that it's not addressing this transitive dependency problem. That doesn't take away from the fact it addresses the direct dependency one though.

because dependencies a,b,c haven't heard of renovate, so don't pin their own dependencies

On this topic, it's a pity that yarn doesn't let you opt in to honouring "pinned" yarn.lock versions of your dependencies. For node.js applications, that seems to be a no-brainer to me because duplicate dependencies in server-side node_modules is not a concern.

For direct dependencies, just run yarn upgrade [DIRECT DEPENDENCY].

I don't think that really helps in the way that you want in this case. Even their example case of yarn upgrade d3-scale@1.0.2 shows that the result in package.json would still be a range dependency.

hutson · 2017-03-25T23:43:33Z

@rarkins thank you for the feedback.

Please see this comment. The numbered list at the bottom is effectively what I would like the renovate behavior to be for my libraries and CLI tools.

hutson · 2017-03-27T00:53:37Z

@rarkins do you have a suggestion for how I could allow for ranged versions to remain in a package.json file, while allowing others to keep the existing behavior?

rarkins · 2017-03-27T03:31:34Z

@hbetts here is one approach:

We have a new config option pinVersions that is defaulted to true, i.e. to match current behaviour
If pinVersions is configured to false, then we default to upgrading semver ranges instead
Later we can make a new option likeupgradeRanges to be configurable too, for people who want to disable that too (e.g. who wish for no changes to package.json to be done by renovate)

The question then is what to do practically with point 2 above. I think for tilde and caret ranges, it's pretty easy to know what the next range should be. For more complex ranges (e.g. anything containing an ||) then we could take one of these two approaches initially:

Do nothing (leave semver unchanged), or
Add the latest release to the range

e.g. let's say the current package.json range for a dependency is ^1.2.1 || ~2.0.0 and the latest version is 2.2.3. In that case, we could augment the semver to be ^1.2.1 || ~2.0.0 || 2.2.3. We know this is unlikely to be merged, but at least it alerts the user that there are new versions above their current range and they can then decide to manually adjust their semver if so. We can of course also adjust the PR description to reflect that the purpose of the PR is to alert them that their existing complex range no longer satisfies the latest release.

Also, over time this allows us to also improve the "intelligence" of the range upgrade logic, e.g. if we see an existing range like ^1.2.1 || ^2.0.0 and then version 3.0.0 is released, then it makes sense for the new range to be ^1.2.1 || ^2.0.0 || ^3.0.0.

ghost · 2017-03-27T19:01:00Z

@rarkins I've added a pinVersions configuration property.

I've also added tests for the versions helper to verify that pinVersions properly effects whether upgrades are pinned or not.

Also, I added the computeRangedVersion function which will generate a ranged version either using a simple ^ or ~, or fallback to appending the new version onto an existing complex range using ||. (computeRangedVersion can be extended later to be more intelligent about the range pattern used.)

This pull request, and it's code, is ready for your review, and hopefully, acceptance.

rarkins · 2017-03-28T13:37:32Z

@destroyerofbuilds I'm trying to work out what's the logical best way to achieve this, because getting the range groupings right needs more than the simple linear approach when pinning versions.

For example if the current version range is ~1.0.0 but versions 1.1.0, 1.2.0, 1.3.0and1.3.1have been released, then I think the desired output would be~1.3.0, right? i.e. you want to find the highest "range group" (1.3.x) but the lowest version that satisfies that group (1.3.0`).

Alternatively, do you think anyone would ever want us to raise a separate PR for each range, like we do for each major version? e.g. they'd want separate PRs for ~1.1.0, ~1.2.0 and ~1.3.0 in the above example?

Because this logic diverges from the linear logic of pinned versions, and because this functionality would be per-package.json and not per-dependency, my instinct is that we might want to create a new determineSemverUpgrades instead of overriding the existing determineUpgrades.

hutson · 2017-03-29T01:45:23Z

needs more than the simple linear approach when pinning versions.

Agreed. You're following example was spot on in that regard.

the lowest version that satisfies that group

Agreed.

the desired output would be~1.3.0, right

That seems reasonable. This pull request handles at least the ~ part for a new minor, just not the second part about the lowest version that satisfies that range.

Alternatively, do you think anyone would ever want us to raise a separate PR for each range, like we do for each major version

I don't know. Seems like doing that would be consistent with the way renovate approaches major versions.

my instinct is that we might want to create a new determineSemverUpgrades

I don't really have anything to add for that proposal. I agree that the logic may seriously complicate what is an already working upgrade function, but then, there's a lot of overlap as well.

rarkins · 2017-03-28T12:40:26Z

test/helpers/versions.spec.js

+    });
+    it('complex range should append pinned version to existing range', () => {
+      versionsHelper.computeRangedVersion('~1.0.0 || ^2.0.0', '2.2.3')
+        .should.eql('~1.0.0 || ^2.0.0 || 2.2.3');


This one doesn't make sense to me because 2.2.3 already satisfies the existing range of ~1.0.0 || ^2.0.0

I agree. This particular scenario would never happen. I've adjusted the test to use version ranges that make sense.

rarkins · 2017-03-28T12:42:56Z

test/helpers/versions.spec.js

@@ -161,4 +240,19 @@ describe('helpers/versions', () => {
      versionsHelper.isPastLatest(qJson, '2.0.3').should.eql(true);
    });
  });
+  describe('.computeRangedVersion(currentVersion, newVersion)', () => {


We need to consider the case when there are multiple available versions and not just the next one. e.g. if current version is ^1.0.0 and versions 2.0.0 and 2.1.0 are available then I think we'd want the result to be ^2.0.0, right?

Agreed.

Though computeRangeVersion is not responsible for deciding which versions to upgrade to. It's only responsible for deciding what range character to place in front of a version.

rarkins · 2017-03-28T12:44:17Z

test/helpers/versions.spec.js

+      versionsHelper.computeRangedVersion('^1.0.0', '2.0.0').should.eql('^2.0.0');
+    });
+    it('should return `~` when single operand and `~` operator is used', () => {
+      versionsHelper.computeRangedVersion('~1.0.0', '2.0.0').should.eql('~2.0.0');


Another test: If current version was ~1.0.0 and versions 1.1.0, 1.2.0, 2.0.0 and 2.0.1 are available then I think the user would want to see two PRs - one for ~1.2.0 and another for ~2.0.0

Agreed.

Though computeRangeVersion is not responsible for deciding which versions to upgrade to. It's only responsible for deciding what range character to place in front of a version.

rarkins · 2017-03-28T13:03:32Z

lib/config/definitions.js

@@ -95,23 +95,23 @@ const options = [
    name: 'commitMessage',
    description: 'Commit message template',
    type: 'string',
-    default: 'Update dependency {{depName}} to version {{newVersion}}',
+    default: 'Update dependency {{depName}} to version {{newVersionRange}}',


I guess we should deprecate newVersion in favour of newVersionRange, i.e. make it overloaded such that it can be a specific version, or it can be any type of complex semver too. I checked the package.json documentation and they describe dependencies as:

Dependencies are specified in a simple object that maps a package name to a version range.

Therefore, newVersionRange seems applicable. Although for consistency it seems like we should rename currentVersion to currentVersionRange too. Should we be super-consistent and also rename nextVersionMajor to nextVersionRangeMajor etc too?

Finally, I propose we do this the following way:

New xVersionRange params replace existing xVersion params

Keep xVersion params in current major version of renovate but remove them next major upgrade

Just for any existing users who are currently overriding any default strings using them, hopefully they check the next major version "release notes" (commit messages)

Alternatively, we could be less semantically correct and keep using xVersion instead of xVersionRange for now (i.e. nextVersion could be 1.1.0 or ^1.1.0) and maybe rename it in next major version to xVersionRange to be more semantically correct.

Also, implied by my above discussion: I don't think we ever need nextVersion and nextVersionRange at the same time in an "upgrade" because you can't be pinning and ranging at the same time.

Please see this comment about the use of newVersionRange - #139 (review)

It was simply to work around how semver works. I have no preference on naming scheme.

rarkins · 2017-03-28T13:07:18Z

lib/config/definitions.js

@@ -131,6 +131,11 @@ const options = [
    description: 'Requested reviewers for Pull Requests (GitHub only)',
    type: 'list',
  },
+  {
+    name: 'pinVersions',
+    description: 'Convert ranged versions to a pinned version',


'Convert ranged versions in package.json to pinned versions'

I've switched it to your wording.

rarkins · 2017-03-28T13:12:47Z

lib/helpers/versions.js

+      // Group updates for the current dependency by the major version.
+      if (!allUpgrades[parsedVersion.major] ||
+          semver.gt(newVersion, allUpgrades[parsedVersion.major].newVersion)) {
+        let upgradeType = 'patch';


I agree that it's time to start properly differentiating between major/minor/patch. Potentially could be pushed into a separate PR though if we wish to simplify this PR's changes even further.

rarkins · 2017-03-30T13:05:33Z

lib/helpers/versions.js

+  // Check whether the current version is pinned, and if so, just return the
+  // new version, which, itself, should be a pinned version number.
+  if (parsedRange.length === 1 && parsedRange[0].operator === undefined) {
+    rangedVersion = newVersion;


Simpler logic if we return newVersion here and the subsequent else if becomes an if ?

I'm simplified the logic.

rarkins · 2017-03-30T13:07:08Z

lib/helpers/versions.js

+    rangedVersion = newVersion;
+
+  // Check whether the existing version range is _simple_, meaning
+  // the range is a single operator, like `^`, `~`, `>=`, etc on a single operand, `1.0.0`.


Is there a definitive list of operators that it could be? ^ and ~ are the simplest, but for example I can't see why we would ever upgrade a simple>= dependency range to anything else?

I'm tempted to be more specific about operators (e.g. whitelist ones we understand, such as ~ and ^) and let everything else fall back to existing || new..

I believe primitive operators, tilde and carrot.

However, I think X-range may also be a single operator, but I'm not sure how to handle that one, so, yeah, I think locking down to ~ and ^ is probably the safe course of action for now.

rarkins · 2017-03-30T13:33:26Z

@destroyerofbuilds I've added some comments as a review. Sorry if this is starting to get "messy" to follow. I'll try to summarise again the key issues we need to make decisions or changes on. If we are in agreement on these points, either of us can make the changes necessary to have this PR merged.

Parsing existing ranges

For this initial approach, I think we may need to enhance the "simple" operator check and instead explicitly check for ones we understand. We understand ~ and ^ and will need advanced logic particularly for ~. If someone has a simple >= or > range then by definition I think we'll never need to upgrade it?

From https://github.com/npm/node-semver, these are "primitive" operators:

< Less than
<= Less than or equal to
Greater than
= Greater than or equal to
= Equal. If no operator is specified, then equality is assumed, so this operator is optional, but MAY be included.

Then we have:
Hyphen Ranges - We can likely add advanced logic for these later, for now we can fall back to ||
X-Ranges - We ideally should "understand" these like we do with ~ and ^
partial version range (e.g. 1 or 1.2) - these are considered implicit X-ranges!

I'm thinking that we maybe initially support only ~ and ^ range types and then create issues for the remainders to have logic added.

newVersion or newVersionRange

newVersionRange is semantically correct, but on that logic then currentVersion should also be currentVersionRange too. I prefer not to change all of these, and prefer not to be semantically inconsistent, so I'm leaning towards overloading the existing newVersion terminology - not adding newVersionRange - and adding this as a todo for the next major release (where we can rename all of them to be semantically correct, if we wish).

Tilde range logic

Should we have one PR per major release (1.x, 2.x, etc) or one PR per upgrade "range" (e.g. for 1.1.x, 1.2.x, and 2.0.x)? I think let's just have one per major release, and if anyone wants more granular ranges then they can raise an issue to show their need.

Should we support the "lowest satisfying version of the maximum range"?
e.g. if current version is ~1.0.0 and versions 1.1.0, 1.2.0 and 1.2.1 are available, then the upgraded range should be ~1.2.0. This complexity is the main reason why I think we need more explicit support of range types rather than the "simple operator" logic that's currently there.

hutson · 2017-04-06T02:53:25Z

Thank you @rarkins for the feedback. I'll do my best to get back to you tomorrow on the discussion points.

ghost

.

ghost · 2017-04-06T19:23:35Z

lib/helpers/versions.js

+
+      // Group updates for the current dependency by the major version.
+      if (!upgrades[parsedVersion.major] ||
+          semver.gt(newVersion, upgrades[parsedVersion.major].newVersion)) {


The reason I keep a separate newVersion and newVersionRange is because of this check. semver.gt only works on a pinned version.

Please feel free to push a commit to replace the use of newVersion and newVersionRange with any naming scheme you feel is appropriate.

sember.gtr actually handles ranges. So that could be used here if someone would like to consolidate the use of newVersion and newVersionRange.

Yes, I think let's consolidate them and utilise the semver.gtr function

ghost · 2017-04-06T19:31:33Z

Parsing existing ranges

I agree with you. This has been changed in the code.

I just read this comment after making my other comment about supporting only ~ and ^.

newVersion or newVersionRange

Please not my comment about semver.

Should we have one PR per major release

I think one per range (depending on whether the range is using ^ or ~).

However, I would not get any current value from that work, and it would mean a greater refactoring of this code, which is not something I want to pursue at this time. At the moment, this change adds value, and enhancing support for more ranges can be evaluated once more people on-board with range support.

Should we support the "lowest satisfying version of the maximum range"?

Yes.

Let me see what I can do there.

ghost · 2017-04-06T21:44:44Z

Should we support the "lowest satisfying version of the maximum range"?

I'm not making headway on coming up with something that supports that feature. I'm sorry.

ghost · 2017-04-06T22:17:37Z

lib/helpers/versions.js

+            isPastLatest(dep, version) && !isPastLatest(dep, workingVersion))
+
+    // Process all remaining versions
+    .reduce((upgrades, newVersion) => {


Okay, even though I'm really out of time to work on the minimum satisfying version feature, one idea I thought of is that you can, right before reduce, use Lodash's groupBy method like .groupBy(semver.major), to group versions by major, and then, if the range is ~, further group by semver.minor.

Then you could use semver's minSatisfying method to find the lowest version in each group after constructing a pseudo version, like, for minor group 5 within major group 4, ~4.5.0.

@destroyerofbuilds I think I know how to solve this using a semver range "hack". We could do it like this:

Find the maximum version for each major version (e.g. existing might be ~1.5.0 and we find 1.7.6). This could be the same logic already used before this PR

Convert this maximum version to a minor semver range by dropping the last digit, e.g. 1.7. In semver speak, 1.7 is equivalent to 1.7.x which for our purposes is equivalent to a tilde range

Use semver's minSatisfying by passing it the full array of versions and 1.7. This usually would then return 1.7.0 but could also return 1.7.1 for example, if 1.7.0 is missing.

In theory we would be iterating over the versions array twice to do this, but I think the performance impact would be so minimal that there's no point worrying about it.

Neat! Sounds good to me!

rarkins · 2017-04-07T06:16:54Z

@destroyerofbuilds I think we agree on what needs to be done now to get this PR completed - let me know if you would like me to make edits directly to your fork/feature.

hutson · 2017-04-08T03:32:01Z

@rarkins I didn't get a chance to start/complete the newVersionRange renaming while at work today.

So please feel free to apply commits on top of the existing work.

rarkins · 2017-04-13T12:11:46Z

Replaced by #155

hutson reviewed Mar 21, 2017

View reviewed changes

ghost commented Mar 24, 2017

View reviewed changes

ghost mentioned this pull request Mar 24, 2017

upgrade-interactive for transitive dependencies yarnpkg/yarn#2543

Open

This was referenced Mar 25, 2017

fix(file): use path.isAbsolute for Windows compatibility #142

Merged

refactor(versions): refactor versions helper #143

Merged

ghost changed the title ~~[WIP] Feat/package/ranges~~ feat/package/ranges Mar 27, 2017

rarkins self-assigned this Mar 28, 2017

rarkins added the in progress label Mar 28, 2017

rarkins reviewed Mar 30, 2017

View reviewed changes

hutson mentioned this pull request Apr 6, 2017

Support for other package managers? #121

Closed

feat(package): keep version ranges

bc39699

ghost commented Apr 6, 2017

View reviewed changes

rarkins closed this Apr 13, 2017

rarkins removed the in progress label Apr 13, 2017

rarkins mentioned this pull request Apr 13, 2017

Support simple range upgrades #155

Merged

feat/package/ranges #139

feat/package/ranges #139

Conversation

ghost commented Mar 20, 2017

Choose a reason for hiding this comment

rarkins commented Mar 21, 2017

ghost commented Mar 21, 2017 • edited by ghost Loading

ghost commented Mar 21, 2017 • edited by ghost Loading

ghost commented Mar 21, 2017 • edited by ghost Loading

rarkins commented Mar 22, 2017

rarkins commented Mar 22, 2017 • edited Loading

ghost commented Mar 22, 2017 • edited by ghost Loading

codecov-io commented Mar 23, 2017 • edited Loading

Codecov Report

ghost commented Mar 23, 2017

rarkins commented Mar 24, 2017

rarkins commented Mar 24, 2017

ghost commented Mar 24, 2017

Choose a reason for hiding this comment

ghost commented Mar 24, 2017

rarkins commented Mar 25, 2017

rarkins commented Mar 25, 2017

hutson commented Mar 25, 2017

hutson commented Mar 27, 2017

rarkins commented Mar 27, 2017

ghost commented Mar 27, 2017

rarkins commented Mar 28, 2017

hutson commented Mar 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarkins commented Mar 30, 2017

Parsing existing ranges

newVersion or newVersionRange

Tilde range logic

hutson commented Apr 6, 2017

ghost left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 6, 2017

ghost commented Apr 6, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rarkins commented Apr 7, 2017

hutson commented Apr 8, 2017

rarkins commented Apr 13, 2017

ghost commented Mar 21, 2017 •

edited by ghost

Loading

ghost commented Mar 21, 2017 •

edited by ghost

Loading

ghost commented Mar 21, 2017 •

edited by ghost

Loading

rarkins commented Mar 22, 2017 •

edited

Loading

ghost commented Mar 22, 2017 •

edited by ghost

Loading

codecov-io commented Mar 23, 2017 •

edited

Loading

ghost commented Apr 6, 2017 •

edited by ghost

Loading