Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve performance and add support for versions like ~1.2.3 and ^1.2.3 #16

Closed
wants to merge 2 commits into from

Conversation

rsp
Copy link
Contributor

@rsp rsp commented Nov 26, 2015

It improves performance by downloading only the data of the version that is requested, instead of downloading data about every published version in the history of the package only to throw most of it away, as was the case before.

The difference is significant, for some modules like express it is 2kB vs 500kB, see:

time curl -s https://registry.npmjs.org/express | wc -c
time curl -s https://registry.npmjs.org/express/4.0.0 | wc -c

This is not only a waste of time and bandwidth from the perspective of the module user but also a significant waste of resources of the npm registry.

This PR also adds support for version numbers like:

  • 1 - get the latest 1.x.x
  • 1.2 - get the latest 1.2.x
  • ^1.2.3 - get the latest 1.x.x but at least 1.2.3
  • ~1.2.3 - get the latest 1.2.x but at least 1.2.3

so this module could be used to test most versions found in package.json dependencies.

It adds tests for the new version formats and makes sure that the data returned after this pull request is still the same as before.

It adds examples to the readme.

It adds support for version numbers like:

* `1` - get the latest `1.x.x`
* `1.2` - get the latest `1.2.x`
* `^1.2.3` - get the latest `1.x.x` but at least `1.2.3`
* `~1.2.3` - get the latest `1.2.x` but at least `1.2.3`

so this module could be used to test most versions found in
package.json dependencies.

It adds tests for the new version formats.

It adds examples to the readme.
This new test makes sure that the data returned after changes
in 5e7ee01 is still the same as before even though the download
is significantly faster and many times smaller:

time curl -s https://registry.npmjs.org/express | wc -c
time curl -s https://registry.npmjs.org/express/4.0.0 | wc -c
@rsp rsp changed the title Add support for versions like ~1.2.3 and ^1.2.3 Add support for versions like ~1.2.3 and ^1.2.3 and improve performance Nov 27, 2015
@rsp rsp changed the title Add support for versions like ~1.2.3 and ^1.2.3 and improve performance Improve performance and add support for versions like ~1.2.3 and ^1.2.3 Nov 27, 2015
var scope = name.split('/')[0];
var url = registryUrl(scope) +
encodeURIComponent(name).replace(/^%40/, '@');
encodeURIComponent(name).replace(/^%40/, '@') +
(!isScoped && version ? '/' + version : '');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Though I agree this is more efficient, please read issue #7.

They are deprecating the version URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@destroyerofbuilds If that is the case (which would be a very strange decision for a good API, I have to admit - the data for all versions will only get bigger over time and eventually it will become a problem) then I guess the PR shouldn't be merged as is. Thanks for pointing it out. Do you maybe have some idea how to get the real version number that would be installed by npm having a string like "~1.2.3" - that wouldn't use the deprecated API that currently supports such versions as part of the URL? Thanks.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use the semver module to match against the list of versions returned.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ghost
Copy link

ghost commented Nov 27, 2015

👎 Because this approach depends on an API end-point that the npm team has indicated may be removed.

rsp added a commit to rsp/package-json that referenced this pull request Nov 28, 2015
It adds support for wildcard, caret and tilde versions
for both scoped and unscoped packages so this module could
be used to test most versions found in package.json dependencies:

* 1 - get the latest 1.x.x
* 1.2 - get the latest 1.2.x
* ^1.2.3 - get the latest 1.x.x but at least 1.2.3
* ~1.2.3 - get the latest 1.2.x but at least 1.2.3

But it keeps downloading the entire data for all versions
from the npm registry - see comments to PR sindresorhus#16 at
sindresorhus#16

It adds tests for the new version formats and makes
sure that the data returned after this pull request
is still the same as before.

It adds examples to the readme.
@rsp
Copy link
Contributor Author

rsp commented Nov 28, 2015

@destroyerofbuilds Thanks for your comments. I prepared a new PR #17 that addresses all your concerns. It adds support for versions like ^1.2.3 and ~1.2.3 for both scoped and unscoped packages while still downloading the entire data for all versions in the package history every time.

@sindresorhus
Copy link
Owner

Wish I could merge this, but don't want to depend on an API the owners has explicitly said will go away at some point.

I hope you can add a comment on https://github.com/npm/public-api/issues/11, though. It's really needed.

@rsp
Copy link
Contributor Author

rsp commented Dec 1, 2015

@sindresorhus OK, I understand. @destroyerofbuilds pointed it out before so I created PR #17 that adds support for versions like ^1.2.3 and ~1.2.3 but without using that API - using the semver module instead. PR #17 adds the functionality to search for wildcard versions and ranges but doesn't improve performance like using the single version API would - e.g. for express you download 500kB just to throw out everything but 2kB immediately, see:

time curl -s https://registry.npmjs.org/express | wc -c
time curl -s https://registry.npmjs.org/express/4.0.0 | wc -c

It may not seem like a huge problem for some people but I think it's a huge waste of resources and it will only get worse with every new version published.

@sindresorhus
Copy link
Owner

@rsp I agree, but the alternative is using an API that might disappear any moment and lacks some features. The better solution is to push npm to add an officially supported API for this as mentioned above.

@rsp
Copy link
Contributor Author

rsp commented Dec 1, 2015

@sindresorhus I agree. I'll comment on npm/public-api#11 that you linked to. Meanwhile there's PR #17 that doesn't use that API but the semver module instead.

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

Successfully merging this pull request may close these issues.

None yet

2 participants