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

Use npm's resolution algorithm #80

Merged
merged 2 commits into from
Feb 2, 2016
Merged

Use npm's resolution algorithm #80

merged 2 commits into from
Feb 2, 2016

Conversation

rstacruz
Copy link
Member

@rstacruz rstacruz commented Feb 2, 2016

@misterbyrne, I've made your module resolution code the default method of finding packages. This is because the one we're using by default, registry.npmjs.org/lodash/4.0.0, is actually an undocumented endpoint (and hence not implemented by gemfury et al).

This instead uses registry.npmjs.org/lodash.

This has the following benefits:

  • no more workarounds for scoped packages
  • support for dist-tags for scoped packages
  • faster gemfury downloads
  • only need to fetch one json per package (eg, installing both lodash@2 and lodash@3 will perform 1 resolution, not one)
  • faster dependency resolution for large packages because of the above (slightly?)

expect a possible slight performance dip for smaller packages, but not by much.

rstacruz added a commit that referenced this pull request Feb 2, 2016
@rstacruz rstacruz merged commit f82dcb8 into master Feb 2, 2016
@indexzero
Copy link
Contributor

Just FYI the problem with this change is that you're going to be downloading an order of magnitude more JSON. e.g. for express

curl -s https://registry.npmjs.org/express -vvvv | grep "Content-Length"
# ...
< Content-Length: 533185

vs.

curl -s https://registry.npmjs.org/express/^3.0.0 -vvvv | grep "Content-Length"
# ...
< Content-Length: 2783

Have you done benchmarks against this change? I suspect this will slow pnpm down a lot.

@rstacruz
Copy link
Member Author

rstacruz commented Feb 2, 2016

Aw man... that's true

@rstacruz
Copy link
Member Author

rstacruz commented Feb 2, 2016

also a shame the registry isn't gzipping that. maybe we can add a check to only do the "optimized" route only for registry.npmjs.org and non-scoped packages...

@indexzero
Copy link
Contributor

@rstacruz how hard would it be to make the resolution-alg a setting? e.g.

pnpm c set resolution-alg semver
# Sets to use URLs like
# https://registry.npmjs.org/express/^3.0.0
pnpm c set resolution-alg npm
# Sets to use URLs like 
# https://registry.npmjs.org/express

@rstacruz
Copy link
Member Author

rstacruz commented Feb 3, 2016

wouldn't it be better if it was automatically inferred for you? it's only supported for registry.npmjs.org and for non-scoped packages; 3rd-party registries are likely to not support it.

@indexzero
Copy link
Contributor

Well it's hard to say which registries support it and which don't. e.g. if GemFury starts implementing it, how do I tell the client to start using it?

@rstacruz
Copy link
Member Author

rstacruz commented Feb 3, 2016

hm, fair i guess, maybe even an npm-style //gemfury.io/:resolution-mode config setting to whitelist certain registries.

@indexzero
Copy link
Contributor

That would be rad. Anything to fallback to the faster downloads.

@misterbyrne
Copy link
Contributor

The thing is that gem fury supports the undocumented requests (by proxy) for anything public. The ratio of private/public packages is pretty low in most cases so allowing gem fury to 404 and failing over to the npm style request then seems perfectly okay to me - faster even because of the reduced payload size for most responses (public packages).

Maybe it would be better to always use the npm style resolution for dist tags (to avoid scope workarounds etc). And use the faster undocumented requests for everything else - allowing a 404 to result in a fallback to the npm style resolution. Sounds like a win-win to me :)

Also if bandwidth is an issue, registries do etags properly so you could cache on disk like npm does and include the relevant etag in any subsequent requests - the resulting 304 will have no body!

@zkochan zkochan deleted the kosher-resolution branch September 3, 2016 20:30
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.

3 participants