Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Explicitly pass in registry. #46

Merged
merged 14 commits into from

3 participants

@othiym23
Owner

This should be considered a WIP, but this piece of work is probably sufficient to get the next piece of npm work down. Removes all calls to this.conf.get('registry') in favor of explicitly passing in the registry on API calls, by requiring fully-qualified URLs.

Also adds (still somewhat rudimentary) support for npm:// URLs. Needs more tests, probably.

lib/get.js
((77 lines not shown))
}.bind(this))
}
-function requestAll (cache, cb) {
+function requestAll (uri, cache, cb) {
+ console.log(cache)
@isaacs Owner
isaacs added a note

Should probably be this.log("info", cache)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/request.js
((20 lines not shown))
+// 2. send authorization
+// 3. content-type is 'application/json' -- metadata
+function regRequest (method, uri, options, cb_) {
+ assert(uri, "must pass resource to load")
+ assert(cb_, "must pass callback")
+
+ options = options || {}
+ var nofollow = !options.follow
+ var etag = options.etag
+ var what = options.body
+
+ var parsed = url.parse(uri)
+
+ if (parsed.protocol === "npm") {
+ parsed.protocol = "https"
+ this.conf.set("always-auth", true)
@isaacs Owner
isaacs added a note

I don't think that the setting should be done this way. Instead, it should simply set a flag for this request to send auth. If other requests come by that are not npm://, then we don't want to send auth unnecessarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
lib/request.js
((32 lines not shown))
+
+ if (parsed.protocol === "npm") {
+ parsed.protocol = "https"
+ this.conf.set("always-auth", true)
+ }
+
+ var where = parsed.pathname
+ if (parsed.search) {
+ where = where + parsed.search
+ parsed.search = ""
+ }
+ parsed.pathname = "/"
+ this.log.verbose("request", "where is", where)
+
+ var registry = url.format(parsed)
+ this.log.verbose("request", "registry is", registry)
@isaacs Owner
isaacs added a note

drop the "is"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs isaacs commented on the diff
lib/unpublish.js
@@ -41,7 +40,7 @@ function unpublish (name, ver, cb) {
// if it was the only version, then delete the whole package.
if (!Object.keys(versions).length) {
this.log.info("unpublish", "No versions remain, removing entire package")
- return this.request("DELETE", name+"/-rev/"+data._rev, cb)
+ return this.request("DELETE", uri + "/-rev/" + data._rev, null, cb)
@isaacs Owner
isaacs added a note

I think we actually don't need to send the _rev any more, because the rewrite passes this to the "delete" update function that takes care of it.

Not relevant to this changeset, but a note for perhaps a future enhancement. We can just send the DELETE, and then treat 404 as a success.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@isaacs
Owner

A few minor comments, but this is mostly looking great. Fix those few things, and merge at will.

I think we will have to bump the major version for this, though, because the API change is a pretty significant breakage.

@isaacs
Owner

Updating docs would be a good addition, as well. The added test coverage is very nice.

@othiym23 othiym23 merged commit 60639c1 into from
@othiym23 othiym23 referenced this pull request from a commit
@othiym23 othiym23 explicitly pass in registry (#46) 47a9806
@othiym23 othiym23 deleted the branch
@dylang

I'm guessing this was done to make it easier to pass in the registry you want to use, but I feel that it making the resulting use of this client more complex.

For example, we need npmConf to create a new client, and we now need to keep around npmConf (or at least the registry uri) for each time we use the client.

Maybe I missed something.

@othiym23
Owner

The only thing you're missing is internal conversations within npm, Inc. The eventual goal is to have npm-registry-client be a pure set of API calls, where each call made with all the configuration information it needs to complete. Right now npm-registry-client lives in an uneasy, twilit world between being an extracted chunk of code from the main npm code base and something that stands alone.

However, extracting all that configuration information out is a large job, and we need to get the basics of scoped modules up and running. This is the piece that was most important for that work, so this is the piece we did right now. I have the corresponding set of changes mostly ready to go over in the npm CLI, and you're right – it's a little unwieldy right now. My intention is to keep pushing at reworking this until it's eventually a clean, standalone API. It may take a little while, though. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 6, 2014
  1. @othiym23
  2. @othiym23

    less noisy tests

    othiym23 authored
  3. @othiym23
  4. @othiym23
  5. @othiym23

    test/fixtures/server.js -> test/lib/server.js

    othiym23 authored
    While I was at it, standardized imports and moved everything to use
    double quotes.
  6. @othiym23

    Add test for bugs endpoint.

    othiym23 authored
  7. @othiym23
  8. @othiym23

    actually test lib/stars.js

    othiym23 authored
    ...and assert that the required callback is there.
  9. @othiym23
  10. @othiym23

    deal with npm:// URLs

    othiym23 authored
  11. @othiym23

    simplify .get()

    othiym23 authored
  12. @othiym23

    fix fetching of /-/all

    othiym23 authored
  13. @othiym23
  14. @othiym23
Something went wrong with that request. Please try again.