feat: use flexsearch for all local searches #997

konrad · 2021-11-13T22:55:33Z

konrad commented

2021-11-13 22:55:33 +00:00

This PR introduces flexsearch when searching for labels, lists and namespaces locally.

To discuss:

There's quite a bit of duplication I created here, not sure how to handle this in a better way.

Also I noticed that, while pretty similar, all three (labels, namespaces, lists) handle storing items in vuex slightly different. Labels are stored in a labels object in the state with the id as key, lists are stored directly in the state without any object in the middle of it and namespaces are stored in a namespaces array.

That has grown historically, but I think it should be refactored at some point.

This PR introduces [flexsearch](https://github.com/nextapps-de/flexsearch) when searching for labels, lists and namespaces locally. #### To discuss: There's quite a bit of duplication I created here, not sure how to handle this in a better way. Also I noticed that, while pretty similar, _all three_ (labels, namespaces, lists) handle storing items in vuex slightly different. Labels are stored in a `labels` object in the state with the id as key, lists are stored directly in the state without any object in the middle of it and namespaces are stored in a `namespaces` array. That has grown historically, but I think it should be refactored at some point.

konrad added 3 commits 2021-11-13 22:55:34 +00:00

feat: use flexsearch to search for labels f805907213

feat: use flexsearch to search for lists locally aadee106ca

feat: use flexsearch to search for namespaces locally

continuous-integration/drone/pr Build is failing

Details

1ae6318a59

konrad commented

2021-11-13 22:56:00 +00:00

@dpschen Do you maybe have an idea how to handle this in a good way?

dpschen requested changes 2021-11-14 15:55:21 +00:00

dpschen left a comment

In general:

Flexsearch seems like a nice lib!
ID as key is usually the way to go. With repetition of the id inside the object. I guess we should change that. The question is when :D

What I don't like:

flexsearch builds another index basically a repetition of some of the info in the store. But I guess there is nothing we can do about that.
we add new complexity that makes it harder to improve and simplify the general architecture.

In general: - Flexsearch seems like a nice lib! - ID as key is usually the way to go. With repetition of the id inside the object. I guess we should change that. The question is when :D What I don't like: - flexsearch builds another index basically a repetition of some of the info in the store. But I guess there is nothing we can do about that. - we add new complexity that makes it harder to improve and simplify the general architecture.

src/helpers/labels.ts Outdated

						
				@ -29,0 +31,4 @@

					})

					return resultIds

						.filter((value, index, self) => self.indexOf(value) === index && !labelIdsToHide.includes(value))

dpschen commented

2021-11-14 15:36:51 +00:00

Use includes()

Use `includes()`

konrad commented

2021-11-14 16:24:08 +00:00

How? I'm using this to filter out duplicate Ids.

dpschen commented

2021-11-14 16:30:28 +00:00

Ignore. Sry, I misread

dpschen marked this conversation as resolved

src/indexes/labels.ts Outdated

						
				@ -0,0 +1,19 @@

				import {createNewIndex, withId} from '@/indexes/index'

dpschen commented

2021-11-14 14:12:46 +00:00

In these three files the duplication can be avoided by creating a factory function.
Something like:

// createNewIndexer.js
import {createNewIndex, withId} from '@/indexes/index'


export function createNewIndexer() {
    function add(item: withId) {
        return index.add(label.id, label)
    }

    function remove(item: withId) {
        return index.remove(label.id)
    }

    function update(item: withId) {
        return index.update(label.id, label)
    }

    function search(item: withId) {
        return index.search(query)
    }
    
    return {
    	add,
        remove,
        update,
        search
    }
}

and then:

// labels.js
import { createNewIndexer } from './createNewIndxer'

export createNewIndexer()

In these three files the duplication can be avoided by creating a factory function. Something like: ```js // createNewIndexer.js import {createNewIndex, withId} from '@/indexes/index' export function createNewIndexer() { function add(item: withId) { return index.add(label.id, label) } function remove(item: withId) { return index.remove(label.id) } function update(item: withId) { return index.update(label.id, label) } function search(item: withId) { return index.search(query) } return { add, remove, update, search } } ``` and then: ```js // labels.js import { createNewIndexer } from './createNewIndxer' export createNewIndexer() ```

dpschen commented

2021-11-14 15:39:20 +00:00

On another thought:
The createNewIndexer should probably just be defined inside the index file already

On another thought: The `createNewIndexer` should probably just be defined inside the index file already

konrad commented

2021-11-14 16:34:42 +00:00

That sounds like a really good idea. I've moved it all to a factory function.

konrad marked this conversation as resolved

src/store/modules/labels.js Outdated

						
				@ -3,6 +3,7 @@ import {setLoading} from '@/store/helper'

				import {success} from '@/message'

				import {i18n} from '@/i18n'

				import {getLabelsByIds, filterLabelsByQuery} from '@/helpers/labels'

				import {add, remove, update} from '../../indexes/labels'

dpschen commented

2021-11-14 14:13:27 +00:00

use @

use `@`

konrad commented

2021-11-14 16:19:10 +00:00

Done.

konrad marked this conversation as resolved

src/store/modules/lists.js Outdated

						
				@ -35,2 +39,4 @@

							return typeof list === 'undefined' ? null : list

						},

						searchList: state => (query, includeArchived = false) => {

							if (query === '' || query === null) {

dpschen commented

2021-11-14 15:40:18 +00:00

Don' repeat all the search validation. Instead put that in the search function.
(you repeated this validatin multiple times)

Don' repeat all the search validation. Instead put that in the search function. (you repeated this validatin multiple times)

konrad commented

2021-11-14 16:39:00 +00:00

In the indexer module? The problem here is it does not just return an array, but another object with the results in them (coming from flexsearch). Should I recreate that?

dpschen commented

2021-11-14 16:43:17 +00:00

I meant putting the query check in the search function that you create =)

konrad commented

2021-11-14 16:48:05 +00:00

Just to clarify, here? https://kolaente.dev/vikunja/frontend/src/branch/feature/flexsearch/src/indexes/index.ts#L39

👍 1

dpschen commented

2021-11-14 16:50:37 +00:00

Yes!

konrad commented

2021-11-14 16:53:11 +00:00

Okay, but what should I return if the query is empty? I can't really return an empty array because the caller expects the object with the results from flexsearch.

Or should I return null and check that in the caller?

Okay, but what should I return if the query is empty? I can't really return an empty array because the caller expects the object with the results from flexsearch. Or should I return `null` and check that in the caller?

dpschen commented

2021-11-14 17:01:16 +00:00

Return null (I would have used undefined, but after thinking about it null seems to make more sense…?).

After the call of search() you usually iterate over the results. You can use optional chaining to check is you can do that (result?.map( [...] ) || FALLBACK)

Return `null` (I would have used `undefined`, but after thinking about it `null` seems to make more sense…?). After the call of `search()` you usually iterate over the results. You can use optional chaining to check is you can do that (`result?.map( [...] ) || FALLBACK`)

dpschen commented

2021-11-14 17:01:28 +00:00

Is that what you mean?

konrad commented

2021-11-14 17:28:07 +00:00

yeah pretty much.

konrad commented

2021-11-14 18:13:54 +00:00

Moved the check and simplified accessing it.

konrad marked this conversation as resolved

src/store/modules/lists.js Outdated

						
				@ -37,0 +51,4 @@

							})

							return resultIds

								.filter((value, index, self) => self.indexOf(value) === index && value > 0)

dpschen commented

2021-11-14 14:01:49 +00:00

use includes()

use `includes()`

konrad marked this conversation as resolved

src/store/modules/namespaces.js Outdated

						
				@ -100,0 +112,4 @@

							const namespacesFromIndex = search(query)

							const resultIds = []

							namespacesFromIndex.forEach(r => {

dpschen commented

2021-11-14 14:04:57 +00:00

Use .map()

Use `.map()`

konrad commented

2021-11-14 16:22:17 +00:00

Done.

konrad marked this conversation as resolved

src/store/modules/namespaces.js Outdated

						
				@ -100,0 +117,4 @@

							})

							return resultIds

								.filter((value, index, self) => self.indexOf(value) === index && value > 0)

dpschen commented

2021-11-14 14:03:48 +00:00

Use .includes()

Use `.includes()`

konrad marked this conversation as resolved

src/store/modules/namespaces.js Outdated

						
				@ -100,0 +119,4 @@

							return resultIds

								.filter((value, index, self) => self.indexOf(value) === index && value > 0)

								.map(id => {

									const namespaceIndex = state.namespaces.findIndex(n => n.id === id)

dpschen commented

2021-11-14 14:03:30 +00:00

Use .find()

Use `.find()`

konrad commented

2021-11-14 16:22:08 +00:00

Done.

konrad marked this conversation as resolved

konrad added 1 commit 2021-11-14 16:19:06 +00:00

chore: use @

continuous-integration/drone/pr Build is failing

Details

b0388407c2

konrad added 1 commit 2021-11-14 16:22:05 +00:00

chore: simplify searching for a namespace

continuous-integration/drone/pr Build is failing

Details

b9ccbb16e4

konrad added 1 commit 2021-11-14 16:34:12 +00:00

chore: use factory function to avoid duplication

continuous-integration/drone/pr Build is failing

Details

6d3eb97d3c

konrad added 1 commit 2021-11-14 16:38:16 +00:00

chore: simplify getting result ids

continuous-integration/drone/pr Build is failing

Details

77b91e5181

konrad added 1 commit 2021-11-14 16:54:18 +00:00

chore: move creating a new index

continuous-integration/drone/pr Build is failing

Details

2f9701f405

konrad added 1 commit 2021-11-14 18:10:45 +00:00

chore: move empty query to search method

continuous-integration/drone/pr Build is failing

Details

b6113cfba2

konrad added 1 commit 2021-11-14 18:13:43 +00:00

chore: simplify checking for empty search results

continuous-integration/drone/pr Build is failing

Details

9776b58efc

konrad added 1 commit 2021-11-14 18:15:23 +00:00

Merge branch 'main' into feature/flexsearch

continuous-integration/drone/pr Build is failing

Details

6dcefcecd1

# Conflicts:
#	package.json

konrad added 1 commit 2021-11-14 18:36:25 +00:00

fix: label tests

continuous-integration/drone/pr Build is passing

Details

ad6b3222d0

frederick commented

2021-11-14 18:43:53 +00:00

Hi konrad!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://997-featureflexsearch--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi konrad! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://997-featureflexsearch--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.

dpschen requested changes 2021-11-14 19:06:01 +00:00

src/helpers/labels.ts Outdated

						
				@ -27,2 +26,2 @@

							return !labelIds.includes(id) && title.toLowerCase().includes(labelQuery)

						})

					return search(query)

							?.map(r => r.result)

dpschen commented

2021-11-14 18:58:30 +00:00

Use flatMap.

konrad commented

2021-11-14 19:26:23 +00:00

Oh, I didn't know that exists. Done.

konrad marked this conversation as resolved

src/store/modules/labels.js

						
				@ -5,1 +5,4 @@

				import {getLabelsByIds, filterLabelsByQuery} from '@/helpers/labels'

				import {createNewIndexer} from '@/indexes'

				const {add, remove, update} = createNewIndexer('labels', ['title', 'description'])

dpschen commented

2021-11-14 19:00:09 +00:00

I like that =)

🎉 1

konrad marked this conversation as resolved

src/store/modules/namespaces.js Outdated

						
				@ -47,3 +55,4 @@

						removeNamespaceById(state, namespaceId) {

							for (const n in state.namespaces) {

								if (state.namespaces[n].id === namespaceId) {

									remove(state.namespaces[n])

dpschen commented

2021-11-14 19:04:55 +00:00

Picky: remove after splice (if why ever splice fails remove will correctly not be reached

Picky: `remove` after splice (if why ever splice fails `remove` will correctly not be reached

konrad commented

2021-11-14 19:27:01 +00:00

Done.

konrad marked this conversation as resolved

src/store/modules/namespaces.js Outdated

						
				@ -100,0 +112,4 @@

									.flat()

									.filter((value, index, self) => self.indexOf(value) === index && value > 0)

									.map(id => {

										const namespace = state.namespaces.find(n => n.id === id)