Add full text search #414
Add full text search #414
Conversation
* Adds a new sqlite table that indexes modules names * Adds `search` command to search for modules
lib/ExportsStorage.js
Outdated
| @@ -192,6 +203,17 @@ export default class ExportsStorage { | |||
| reject(err); | |||
| return; | |||
| } | |||
|
|
|||
| if (!additional) { | |||
There was a problem hiding this comment.
I had to read the code a few times to understand what this boolean meant. A comment here could probably make things clearer. 🤔
lib/ExportsStorage.js
Outdated
| @@ -52,6 +52,17 @@ export default class ExportsStorage { | |||
|
|
|||
| this.db.run( | |||
| ` | |||
| CREATE VIRTUAL TABLE exports_fts USING fts4( | |||
There was a problem hiding this comment.
Can you elaborate a little why the virtual table is necessary here? I could see this being implemented using "simple" WHERE name LIKE 'foobar%' as well. The redundancy is probably okay, I'm asking more because I'm (passively) searching for alternatives to sqlite3. Multiple people have reported that it's hard to install (e.g. Galooshi/atom-import-js#15, Galooshi/atom-import-js#18) and it would be easier if there was a "pure" javascript alternative.
There was a problem hiding this comment.
sqlite's full text search requires a virtual table and using full text search performs better than LIKE 'foo%'. I'm not too familiar with sqlite, but it's possible we don't need a separate table at all, but I figured this way I don't break anything.
lib/ExportsStorage.js
Outdated
|
|
||
| if (!additional) { | ||
| this.db.run( | ||
| 'INSERT INTO exports_fts VALUES (?, ?, ?, ?)', |
There was a problem hiding this comment.
fts is for full text search, right? I think it would be okay to spell that out instead of using the abbreviation.
lib/ExportsStorage.js
Outdated
| return; | ||
| } | ||
| resolve( | ||
| rows.map(({ name, path, isDefault, packageName }) => ({ |
There was a problem hiding this comment.
If you wanted to DRY up things, you could introduce a normalization function (normalizeRowData?) and use it here and inside the get() method.
lib/ExportsStorage.js
Outdated
| ...defaultNames.map(name => | ||
| this._insert({ name, pathToFile, isDefault: true, packageName })), | ||
| ...defaultNames.map((defaultName: string | Object) => { | ||
| const defaultNameObj = typeof defaultName === 'string' ? { name: defaultName, additional: false } : defaultName; |
There was a problem hiding this comment.
This line looks a little defensive. Can we make sure that we only pass in objects instead?
| @@ -19,7 +19,6 @@ beforeEach(() => { | |||
| it('returns matching modules', () => findJsModulesFor( | |||
| new Configuration('./bar.js'), | |||
| 'foo', | |||
| './bar.js', | |||
…ring, update tests
|
Ready to go? One thing that crossed my mind when looking at the deoplete plugin is that, since you're using the command-line tool directly, there's a dependency on a warm cache. If people use your plugin without also using vim-import-js (actively), the items they need won't be in import-js storage. If you wanted to fix that you could use a daemon |
|
Should be good to go now! For the deoplete plugin, I should probably call |
| // currently just casts `isDefault` to boolean | ||
| return rows.map(({ isDefault, ...other }) => ({ | ||
| isDefault: !!isDefault, | ||
| ...other, |
|
Yeah, that sounds like the right thing. You'd also have to add search to the list of commands in |
|
I plan on releasing this tomorrow after some manual testing locally. Thanks again for the contribution @billyvg! |
|
This is perfect timing. I was just chatting with @dabbott about using import-js with autocomplete. It would be great if it added the import after selecting the item from the autocomplete list. |
|
I'm very excited about this as well. If I read the source for the deoplete plugin right, I believe it did import the variable as well. |
|
Yep the deoplete plugin will add the import statement - though there are some small annoying bugs with it. |
I would like to have this feature so that autocomplete plugins (i.e. deoplete) can be supported. The workflow would be more streamlined this way.
Edit:
Here is the deoplete plugin