API-based npm search command #15481

zkat · 2017-01-14T10:09:43Z

Yay!

So this PR makes it so the npm CLI actually hits the shiny new search endpoint! Default npm search on the main registry should be blazing fast now.

No. Seriously:

The search system still falls back to cache or /-/all-based searches for registries that don't support this endpoint, and if you happen to have a cache while running offline, it'll use that cache with classic search.

To-Do

Note: The PR is basically ready to review of the bulk of the changes, but below are a couple of items that are still pending.

unit tests for esearch
get rid of gunzip-maybe because it adds almost 2MB
Figure out what to do about --searchlimit when using all-package-search
unit tests for all-package-search
Check what negation is gonna be like
figure out whether/how to add sorting to esearch

coveralls · 2017-01-14T10:26:27Z

Coverage increased (+0.07%) to 85.929% when pulling 9368967 on zkat/fastsearch into 58aa3b6 on latest.

bcoe · 2017-01-16T00:36:10Z

@zkat this is amazing; I will read through the pull while OSSing over the next couple days. We should get you added to the npms.io slack too; so we can discuss adding --searchexclude.

CC: @satazor, @atduarte, how cool is this?!

bcoe

this is looking pretty solid to me; I like the how you fallback to the old search.

One thing comes to mind, because browserify-zlib check in there fixture directory, we end up with some pretty odd artifacts in the bundled dependencies. e.g., this picture of a person:

https://github.com/npm/npm/blob/936896722f6ba18a90120c7fac475ddecfecd9cd/node_modules/gunzip-maybe/node_modules/browserify-zlib/test/fixtures/person.jpg

It might be worth opening an issue, or simply sending a pull-request to browserify-zlib that removes test files from the package.

bcoe · 2017-01-16T00:44:14Z

lib/search/esearch.js

+function esearch (opts) {
+  var stream = ms.through.obj()
+
+  mapToRegistry('-/v1/search', npm.config, function (er, uri, auth) {


this warms my heart 👍

satazor · 2017-01-16T00:48:26Z

This is definitively cool! One question: does it support qualifiers? e.g.: npm search author:sindresorhus

@bcoe I guess you have permissions to invite @zkat to our slack.

zkat · 2017-01-16T07:44:54Z

@satazor all the current search syntax is supported because we pretty much pass arguments directly to the endpoint. You can do maintainer:zkat just fine. :)

EDIT: oops. You already talked about that! I'd love an invite to the Slack :)

coveralls · 2017-01-16T16:54:51Z

Coverage decreased (-0.06%) to 85.798% when pulling 5b6eb36 on zkat/fastsearch into 58aa3b6 on latest.

atduarte · 2017-01-16T18:53:04Z

Awesome! :D

coveralls · 2017-01-18T11:31:46Z

Coverage increased (+0.09%) to 85.95% when pulling a514935 on zkat/fastsearch into 58aa3b6 on latest.

bcoe · 2017-01-18T16:11:59Z

🎉 🎈 this is really exciting.

iarna

This looks great! Can't wait to have it!

I had a couple of nits but nothing blocking.

iarna · 2017-01-19T19:17:11Z

lib/search/esearch.js

+    streaming: true
+  }
+  var q = buildQuery(opts)
+  npm.registry.request(uri + '?text=' + encodeURIComponent(q) + '&size=' + opts.limit, params, function (er, res) {


Absurdly picky style note: If we use err then standard can complaint if we fail to check it. I've been trying to get in the habit of using err instead of er so I can have that validation.

iarna · 2017-01-19T19:18:12Z

lib/search/esearch.js

+    //        Modify how `res` is used here at your own risk.
+    var entryStream = ms.pipeline.obj(
+      res,
+      ms.through(function (chunk, enc, cb) {


This is just to turn res into a modern stream?
I think usingnew stream.PassThrough() here would be less ambiguous..

No it's just literally that weird and there's something about using this particular type of passthrough that worked through an issue I was having. I didn't do the due diligence of figuring out what was happening back then (during the original rewrite, which this code is based on). So maybe I'll just have to do it but siiiiiggghhhh

iarna · 2017-01-23T22:00:28Z

test/tap/search.js

 }

 function cleanup () {
+  server.done()


why is this in a different place than server.close()?

close shuts down the server. done does missing-request checks while letting us reuse the same server (so, it errors if we don't hit the mocked endpoint at all)

iarna · 2017-01-23T22:03:24Z

test/tap/search.esearch.js

@@ -0,0 +1,192 @@
+var common = require('../common-tap.js')


search.esearch.js? Not search-esearch.js?

That's just me being inconsistent. I've been using the former with pacote and cacache and it just feels nicer for hierarchies of tests in a single flat dir

coveralls · 2017-01-25T13:34:38Z

Coverage increased (+0.08%) to 85.94% when pulling d93f97d70eda95558ac44a345f58d7899980e8f8 on zkat/fastsearch into 04fca22 on release-next.

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

@zkat

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

coveralls · 2017-01-25T14:11:26Z

Coverage increased (+0.09%) to 85.952% when pulling eb64046 on zkat/fastsearch into 04fca22 on release-next.

zkat added the review label Jan 14, 2017

zkat requested review from bcoe and iarna January 14, 2017 10:09

bcoe approved these changes Jan 16, 2017

View reviewed changes

zkat added the in-progress label Jan 16, 2017

zkat removed the in-progress label Jan 18, 2017

iarna approved these changes Jan 23, 2017

View reviewed changes

zkat force-pushed the zkat/fastsearch branch from a514935 to d93f97d Compare January 25, 2017 13:03

zkat added merge-to-latest and removed review labels Jan 25, 2017

zkat added this to the next milestone Jan 25, 2017

zkat changed the base branch from latest to release-next January 25, 2017 13:04

zkat added 5 commits January 25, 2017 14:42

utils: added an internal gunzip-maybe utility

cc12b26

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

search: added support for endpoint-based searching

447fc58

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

config: added config for searchlimit

bc6a5bc

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

test: tests for new search code

9ed7220

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

doc: documented new searchlimit option

eb64046

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

zkat force-pushed the zkat/fastsearch branch from d93f97d to eb64046 Compare January 25, 2017 13:44

zkat merged commit c56618c into release-next Jan 25, 2017

zkat added a commit that referenced this pull request Jan 25, 2017

utils: added an internal gunzip-maybe utility

ce3ca51

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

zkat added a commit that referenced this pull request Jan 25, 2017

search: added support for endpoint-based searching

e322932

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

zkat added a commit that referenced this pull request Jan 25, 2017

config: added config for searchlimit

a53055e

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

zkat added a commit that referenced this pull request Jan 25, 2017

test: tests for new search code

a1f4365

PR-URL: #15481 Credit: @zkat Reviewed-By: @iarna Reviewed-By: @bcoe

zkat mentioned this pull request Feb 14, 2017

deps: upgrade npm to 4.2.0 nodejs/node#11389

Closed

4 tasks

API-based npm search command #15481

API-based npm search command #15481

Uh oh!

Conversation

zkat commented Jan 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To-Do

Uh oh!

coveralls commented Jan 14, 2017

Uh oh!

bcoe commented Jan 16, 2017

Uh oh!

bcoe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

satazor commented Jan 16, 2017

Uh oh!

zkat commented Jan 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Jan 16, 2017

Uh oh!

atduarte commented Jan 16, 2017

Uh oh!

coveralls commented Jan 18, 2017

Uh oh!

bcoe commented Jan 18, 2017

Uh oh!

iarna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jan 25, 2017

Uh oh!

coveralls commented Jan 25, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zkat commented Jan 14, 2017 •

edited

Loading

zkat commented Jan 16, 2017 •

edited

Loading