-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(search): use dexie.js instead of localStorage #2464
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sy-records --
I think this is a great start and on its own addresses the localStorage limitations we were hoping to address by switching to IndexedDB. Well done.
-
Now that we have a database with rows and columns that we can query, my hope is that we can move away from the localStorage-based technique of storing and parsing one huge JSON string for our search plugin. Doing this adds significant and noticeable blocking tasks to the main thread that negatively impact our user experience and performance ratings (e.g., Lighthouse) which can also have an impact on SEO (even for SPAs like Docsify).
As an example, the docsify.js.org search plugin JSON is 309399 character (~300k). Chrome seems to handle this without issue, but Safari has a very noticeable delay when loading a Docsify site for the first time. The delay in Safari is resolved as soon as I disable the search plugin.
-
I've noticed that if an error occurs during Docsify's initialization it can cause the search plugin to (I think) store invalid data which in turn causes the plugin to not work until its data expires. We should account for this, perhaps by not setting the search data expiration date until after indexing has completed successfully or attempting to remove+rebuild data one time when the plugin encounters an error.
-
See fix(search): clean markdown elements in search contents #2457 for an interestingly related topic: when to process search data (ingress vs egress). CC: @Koooooo-7
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sy-records could you provide more details (thoughts or design) on this changes?
The plain code is hard to see what happens.
Additionally, I hope there could have some benchmark or basic metric to show on what the changes bring.
- Compare the localStorage with the row/column storage in Dexies < 5MB, e.g. our own site.
- The large content performance (5MB --- localStorage limitation, 10MB, or more?) based on current Dexies solution.
Then, we could have a general understanding about the changes for further refinement since our goal is to handle more larger contents and the performance ++.
FYI, Dexies supports the fuzzy search db.table.filter(row => /tom/i.test(row.name))
or the fullText search https://github.com/dexie/Dexie.js/blob/master/samples/full-text-search/FullTextSearch.js and other functions (with some limitations either). Hope it helps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sy-records any updates on this.
As sync with @jhildenbiddle (details in discord)
Could you please have a basic performance test on this, hope it could not that worse then we can merge it and move on.
Besides, I suppose you can add more details either in comments or in this PR about this changes.
It will easily for us to trace the code for debugging and handover since we already have lot of anonymous
codebase.
Thx in advance.
Implemented dexie.js instead of localStorage in this PR. Removed parsing json strings. db.version(1).stores({
search: 'slug, title, body, path, indexKey',
expires: 'key, value',
}); No changes to the search logic, no full-text search using Dexies. Putting indexKey into INDEXES for querying from IndexedDB. And the storage size, which depends on the user's device and browser limitations, can be tesed using it.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I have a little question on the changes storage structure, we can have sync later in more details.
Regarding to the search functionality, it works fine.
@Koooooo-7 -- Can we get your approval so we can merge this PR? |
Hi @jhildenbiddle I approved it already. |
Ha! Apologies, @Koooooo-7. Looks like I am the hold up on this one. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks, @sy-records.
Summary
use dexie.js instead of localStorage.
Maybe we also need to remove JSON parsing?
Related issue, if any:
close #2281
close #1538
What kind of change does this PR introduce?
Feature
For any code change,
Does this PR introduce a breaking change?
No
Tested in the following browsers: