Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Speed up search by not invoking apm #1014

Merged
merged 9 commits into from
Jan 19, 2018
Merged

Speed up search by not invoking apm #1014

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
47bbea4
Remove underscore requirement from install-panel
Ingramz Oct 29, 2017
4239b94
Speed up search by avoiding apm overhead
Ingramz Oct 29, 2017
278c168
:fire: packageManager.search
Ingramz Dec 11, 2017
46a55c2
Request sorted results
Ingramz Jan 4, 2018
e64b9e3
Fix lint
Ingramz Jan 4, 2018
c8efff1
Query packages by relevance, sort by downloads locally
Ingramz Jan 6, 2018
6bb842c
Fix highlighted results not working on search results that contain up…
Ingramz Jan 8, 2018
b1f5fc8
Make search use gzip
Ingramz Jan 9, 2018
7cf0831
Convert regular request to gzip and json, update dependency
Ingramz Jan 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions lib/atom-io-client.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,34 @@ class AtomIoClient

getCachePath: ->
@cachePath ?= path.join(remote.app.getPath('userData'), 'Cache', 'settings-view')

search: (query, options) ->
qs = {q: query}

if options.themes
qs.filter = 'theme'
else if options.packages
qs.filter = 'package'

if options.sort
qs.sort = options.sort

options = {
url: "#{@baseURL}packages/search"
headers: {'User-Agent': navigator.userAgent}
qs: qs
json: true
}

new Promise (resolve, reject) ->
request options, (err, res, body) ->
Copy link
Contributor

@winstliu winstliu Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm unfamiliar with proxying but is there anything special that needs to be done here to avoid proxy issues?

EDIT: Seems like some ongoing proxy work in #1010 that you could look at.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

    if @httpsProxy?
      options.proxy = @httpsProxy
    if @strictSsl is false
      options.rejectUnauthorized = false

is what is needed to be added to the request options when that pull request is merged.

if err
error = new Error("Searching for \u201C#{query}\u201D failed.")
error.stderr = err.message
reject error
else
resolve(
body.filter (pkg) -> pkg.releases?.latest?
.map ({readme, metadata, downloads, stargazers_count}) ->
Object.assign metadata, {readme, downloads, stargazers_count}
)
12 changes: 5 additions & 7 deletions lib/install-panel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

import path from 'path'
import electron from 'electron'
import _ from 'underscore-plus'
import {CompositeDisposable, TextEditor} from 'atom'
import etch from 'etch'

Expand Down Expand Up @@ -207,12 +206,11 @@ export default class InstallPanel {
this.refs.searchMessage.textContent = `Searching ${this.searchType} for \u201C${query}\u201D\u2026`
this.refs.searchMessage.style.display = ''

const opts = {}
opts[this.searchType] = true
opts['sortBy'] = 'downloads'
const options = {sort: {downloads: 'desc'}}
Copy link
Contributor

@winstliu winstliu Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Should just be const options = {sort: 'downloads'}. The atom.io API is documented at https://flight-manual.atom.io/atom-server-side-apis/sections/atom-package-server-api/. Though it seems like the direction parameter is a bit broken for downloads because I can't get it to sort ascending...luckily we sort descending 😀.

Also, seems weird how we sort by downloads. Now that the relevance sorting has been improved maybe we can default to relevance instead, probably in a different PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

const options = {sort: 'downloads'}
screen shot 2018-01-05 at 15 20 09

const options = {sort: {downloads: 'desc'}}
screen shot 2018-01-05 at 15 19 11

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considering sorting was not done before, I am guessing it can be straight omitted and perhaps the reason why it works is because it has no effect (unconfirmed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it was because of the garbage value in sort field why it seemed to work.

However, when properly sorting on serverside vs not sorting at all, we are getting way different results. The default, unsorted search seems to give more relevant results compared to what is popular, but less relevant. I will roll back the sorting part from last night.

options[this.searchType] = true

try {
let packages = (await this.packageManager.search(query, opts)) || []
const packages = (await this.client.search(query, options)) || []
this.refs.resultsContainer.innerHTML = ''
this.refs.searchMessage.style.display = 'none'
if (packages.length === 0) {
Expand All @@ -233,7 +231,7 @@ export default class InstallPanel {
}

highlightExactMatch (container, query, packages) {
const exactMatch = _.filter(packages, pkg => pkg.name === query)[0]
const exactMatch = packages.filter(pkg => pkg.name === query)[0]

if (exactMatch) {
this.addPackageCardView(container, this.getPackageCardView(exactMatch))
Expand All @@ -242,7 +240,7 @@ export default class InstallPanel {
}

addCloseMatches (container, query, packages) {
const matches = _.filter(packages, pkg => pkg.name.indexOf(query) >= 0)
const matches = packages.filter(pkg => pkg.name.indexOf(query) >= 0)

for (let pack of matches) {
this.addPackageCardView(container, this.getPackageCardView(pack))
Expand Down
30 changes: 0 additions & 30 deletions lib/package-manager.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,36 +255,6 @@ class PackageManager
[version] = version.split('-') if typeof version is 'string'
version

search: (query, options = {}) ->
new Promise (resolve, reject) =>
args = ['search', query, '--json']
if options.themes
args.push '--themes'
else if options.packages
args.push '--packages'
errorMessage = "Searching for \u201C#{query}\u201D failed."

apmProcess = @runCommand args, (code, stdout, stderr) ->
if code is 0
try
packages = JSON.parse(stdout) ? []
if options.sortBy
packages = _.sortBy packages, (pkg) ->
return pkg[options.sortBy]*-1

resolve(packages)
catch parseError
error = createJsonParseError(errorMessage, parseError, stdout)
reject(error)
else
error = new Error(errorMessage)
error.stdout = stdout
error.stderr = stderr
reject(error)

handleProcessErrors apmProcess, errorMessage, (error) ->
reject(error)

update: (pack, newVersion, callback) ->
{name, theme, apmInstallSource} = pack

Expand Down
4 changes: 2 additions & 2 deletions spec/install-panel-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ describe 'InstallPanel', ->

describe "searching packages", ->
it "highlights exact name matches", ->
spyOn(@packageManager, 'search').andCallFake ->
spyOn(@panel.client, 'search').andCallFake ->
new Promise (resolve, reject) -> resolve([ {name: 'not-first'}, {name: 'first'} ])
spyOn(@panel, 'getPackageCardView').andCallThrough()

Expand All @@ -72,7 +72,7 @@ describe 'InstallPanel', ->
expect(@panel.getPackageCardView.argsForCall[1][0].name).toEqual 'not-first'

it "prioritizes partial name matches", ->
spyOn(@packageManager, 'search').andCallFake ->
spyOn(@panel.client, 'search').andCallFake ->
new Promise (resolve, reject) -> resolve([ {name: 'something else'}, {name: 'partial name match'} ])
spyOn(@panel, 'getPackageCardView').andCallThrough()

Expand Down
1 change: 0 additions & 1 deletion spec/package-manager-spec.coffee
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ describe "PackageManager", ->

it "handle errors spawning apm", ->
noSuchCommandError = if process.platform is 'win32' then ' cannot find the path ' else 'ENOENT'
waitsForPromise shouldReject: true, -> packageManager.search('test')
waitsForPromise shouldReject: true, -> packageManager.getInstalled()
waitsForPromise shouldReject: true, -> packageManager.getOutdated()
waitsForPromise shouldReject: true, -> packageManager.getFeatured()
Expand Down