-
-
Notifications
You must be signed in to change notification settings - Fork 145
Fix bug where the token separators used in the search component and the search indexer were inconsistent #191
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
Conversation
…he search inder were inconsistent
index.js
Outdated
| projectHref: info && info.browse(), | ||
| deployVersion: 'ADDON_DOCS_DEPLOY_VERSION' | ||
| deployVersion: 'ADDON_DOCS_DEPLOY_VERSION', | ||
| searchTokenSeparator: /\s+/ |
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.
Unless I'm mistaken, a regex won't survive the JSON.stringify pass that happens when config is serialized for the app. Can we add a test to verify this is working?
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.
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.
Added. I'm open to a different location for the test.
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.
It seems like that's working because the search terms aren't being split at all — that means if I search e.g. render multiple with your changes, I get no results, but on the latest deployed version of the site, that returns several options back.
It looks like the regex is being encoded as {} in the serialized config:
| await visit('/sandbox'); | ||
|
|
||
| assert.equal(modulePage.searchResults.items.length, 0, 'no search results shown'); | ||
| await modulePage.fillInSearchQuery('sub-subsection'); |
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.
Oddly enough, the search works as every character of sub-subsection is typed out, except when the query is sub-subsecti or sub-subsectio.
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.
That makes sense to me — Lunr's stemmer likely recognizes ion as a strippable suffix, so it's actually indexing that word as sub-subsect. When you enter sub-subect yourself, that matches directly, and when you enter sub-subsection completely, it gets stemmed the same way, but sub-subsecti and sub-subsectio would both be left alone by the stemmer, so they wouldn't match.
|
Pushed a change to reencode the RegExp as a string, that then gets converted to a RegExp. |
dfreeman
left a comment
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.
Thanks, @antidis!


This meant that words like
docs-snippetwere being indexed asdocsandsnippet, but would be searched for asdocs-snippet.I'm not sure that method of sharing state is particularly great, but we can't
importfromlib/broccoli, orrequirefromaddon/services.There's some good suggestions over here on how search could be further improved with a Lunr pipeline: olivernn/lunr.js#296