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

Use web worker to build Lunr index #1396

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
2 participants
@yeraydiazdiaz
Contributor

yeraydiazdiaz commented Jan 27, 2018

Addresses #1218, #859 and #1127

Reading the discussion on #859 I decided to give the Web Workers API a try and got a working solution using them. In the process I managed to remove require.js and mustache from the dependencies which seemed overkill for our use case.

I'm not completely replacing the previous code since it's possible the user's browser might not the Web Workers API. Based on our target browser versions we can discuss if we load or remove the previous version.

I also realised it'd be quite simple to upgrade to the current version Lunr and maybe include additional language support but would require some changes to the configuration, we can discuss it outside the context of this PR.

@yeraydiazdiaz yeraydiazdiaz force-pushed the yeraydiazdiaz:web-worker-search branch from f8cc9ff to e8118c7 Jan 27, 2018

@yeraydiazdiaz yeraydiazdiaz force-pushed the yeraydiazdiaz:web-worker-search branch from e8118c7 to 1a6b840 Jan 27, 2018

@waylan

This comment has been minimized.

Member

waylan commented Jan 27, 2018

Awesome. I am going to hold off merging this though. This has been part of a larger refactor of search and I'd like to get everything worked out before committing to how any one part is implemented. For example, I didn't merge #1061 because I wanted it to work with the webworker stuff (it's also out-of-date but that's another matter).

My intention is to build a new search plugin and leave the existing "legacy search" plugin as-is. The two can then reside side-by-side.

@yeraydiazdiaz

This comment has been minimized.

Contributor

yeraydiazdiaz commented Jan 28, 2018

That's fine, I was trying to see if the approach would work and how much of the current search funcionality we actually need, turns out it's not a lot.

How would you like to proceed? Feels these changes should be part of #1061 which in turn is part of the larger refactor, but its PR is against master, so I'm a little confused. Is there a branch for this refactor that I'm missing?

@waylan waylan added the Plugin label Jan 28, 2018

@waylan waylan added this to the 1.0.0 milestone Jan 28, 2018

@waylan waylan added the Enhancement label Jan 28, 2018

@waylan

This comment has been minimized.

Member

waylan commented Jan 28, 2018

#1061 was against master before search was broken out into a plugin and was also an experiment to see if the approach would work (it does, although I would do some things a little differently). Today it only exists as a point of reference. I would be more inclined to start working off of this and reimplement #1061 on top of this. In fact, I'm reopening this so it doesn't get lost and forgotten about.

@waylan waylan reopened this Jan 28, 2018

@yeraydiazdiaz

This comment has been minimized.

Contributor

yeraydiazdiaz commented Jan 29, 2018

Sounds good, let me know if you'd like some help with it 👍

@waylan waylan added this to To Do in Refactor search. Jan 31, 2018

@waylan waylan moved this from To Do to Completed in new-search branch in Refactor search. Jan 31, 2018

@waylan

This comment has been minimized.

Member

waylan commented Jan 31, 2018

I took this and re-implemented it as a new plugin in the waylan:new-search branch (see master...waylan:new-search for comparison). I left bea4fcf as a separate commit to show how I implemented the non-worker fallback ('ll probably squash it later).

I should note that all of the URLs need to be relative as the root of the site may not be hosted at the server root (ex: username.github.io/projectname/). This turned out to be the trickiest thing to work around (when loading the index from the worker the JSON files URL needs to be relative to the worker, but when loading from the main thread, the JSON file's URL needs to be relative to the page). Of course, once I had a solution, it was an easy fix, but it may be non-obvious from the code why I did that.

The entire thing is a work in progress. I've updated the project to reflect what has been resolved and what is outstanding. Any feedback and/or improvements are welcome.

@waylan waylan referenced this pull request Feb 27, 2018

Merged

Refactor search plugin #1418

@waylan waylan closed this in #1418 Mar 6, 2018

Refactor search. automation moved this from In Progress to Done Mar 6, 2018

waylan added a commit that referenced this pull request Mar 6, 2018

Refactor search plugin (#1418)
* Use a web worker in the browser with a fallback (fixes #859 & closes #1396).
* Optionally pre-build search index (fixes #859 & closes #1061).
* Upgrade to lunr.js 2.x (fixes #1319).
* Support search in languages other than English (fixes #826).
* Allow the user to define the word separators (fixes #867).
* Only run searches for queries of length > 2 (fixes #1127).
* Remove dependency on require.js, mustache, etc. (fixes #1218).
* Compress the search index (fixes #1128).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment