Skip to content
This repository was archived by the owner on May 29, 2018. It is now read-only.

Conversation

xizhao
Copy link

@xizhao xizhao commented Oct 9, 2014

NPM should by default take the most up-to-date one if it has a range of options from what is supplied.

Also added some if statements to prevent it from crashing in the case where it can't find a matching version.

@samertm
Copy link

samertm commented Oct 9, 2014

Thanks for the PR!

Three comments: first, I think the 'master' branch is broken to begin with regarding the call to npm.commands.view, because when I run this in its own block:

var npm = require("npm");
npm.load(function(err, npm) {
    if (err) {
        console.error("npm.load failed: ", err);
        process.exit(1)
    }

    npm.commands.view(["--silent", "chalk@latest"], function(err,data) {
        if (err) {
            console.log("error occurred while resolving: " + JSON.stringify(err));
        } else if (!data || Object.keys(data).length == 0) {
            console.log("no npm package found with spec " + JSON.stringify(spec));
        } else {
            console.log('here');
            var chooseVersion = Object.keys(data)[0];
            var info = data[chooseVersion];
        }
    });
});

I get the following output:

npm http GET https://registry.npmjs.org/--silent
npm http 404 https://registry.npmjs.org/--silent
error occurred while resolving: {}

According to the docs, the correct way to silence the call is to pass 'true' as the second argument (https://www.npmjs.org/doc/api/npm-view.html). If you want to fix that in this PR too that would be awesome 😄

Second, I don't think Object.keys' order is defined: we shouldn't rely on the last item to be the latest. I don't know if there are any libraries in the javascript world for sorting strings based on the 'largest' version number, or if npm has a function to do that. What do you think?

Third, var info = Object.keys(data).slice(-1)[0]; will set info to a string representing the last key in its enumerable properties, when it's supposed to be the object that's represented by that version. Maybe

var latestVersion = OurAwesomeVersionNumberSortingFunction(Objects.keys(data))[0];
var info = data[latestVersion];

makes more sense.

@xizhao
Copy link
Author

xizhao commented Oct 10, 2014

According to the docs, the correct way to silence the call is to pass 'true' as the second argument (https://www.npmjs.org/doc/api/npm-view.html). If you want to fix that in this PR too that would be awesome 😄

Yup, it is. Both should work depending on your NPM version, though. When I did this, I used:

npm.commands.view(["chalk@latest"], true, function(){});

Second, I don't think Object.keys' order is defined: we shouldn't rely on the last item to be the latest. I don't know if there are any libraries in the javascript world for sorting strings based on the 'largest' version number, or if npm has a function to do that. What do you think?

Technically, it shouldn't be defined, as dictionaries are orderless. However, I think this method is actually pretty reliable since NPM always responds with the version keys in order. The sorting function won't be a simple numerical parse + comparison, though, as version numbers can be anything in the NPM spec including something like 1.0.1-alpha. Luckily, NPM released semver which has rcompare, a compare function that parses and compares NPM version strings for descending sorts. This is used by NPM itself, so we'd only have to do:

var versions = Object.keys(data);
versions.sort(require('semver').rcompare);
var info = versions[0];

@samertm
Copy link

samertm commented Oct 12, 2014

Technically, it shouldn't be defined, as dictionaries are orderless. However, I think this method is actually pretty reliable since NPM always responds with the version keys in order.

Huh, interesting. I think sorting the list is still a good idea, and I think your solution makes sense.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants