-
Notifications
You must be signed in to change notification settings - Fork 335
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: Add "latest" and "related" search. #2055
base: main
Are you sure you want to change the base?
Conversation
There isn't because this is the first time we do something like this. However, since it comes from hexdocs, I believe it is fair to assume a naming scheme.
@ruslandoga, is this something we can change in the indexer?
The reason it returns the same results over and over again is because we break the documentation of a single function into multiple entries, one per h2 to be more precise. The benefit is that we can provide more precise links too. For now, I don't think we dedup them for regular results right? So I would not dedup them here, but we can dedup them later if we want to (we can even render it like Google results, where we show the main entry and below we refer to specific sections in the result). This is fantastic progress. :) |
FYI, in the algolia for https://erlang.org/doc/search we disable |
👋 I couldn't find any options to make highlighting ignore the token separators. I'll try asking Typesense people if it's supported or planned to be supported. For now, I think we might need to roll our own highlighter, possibly similar to the one used in autocomplete: ex_doc/assets/js/autocomplete/suggestions.js Lines 289 to 311 in 51cd422
Removing |
const searchNodes = getSearchNodes() | ||
|
||
if (['related', 'latest'].includes(queryType) && searchNodes.length > 0) { | ||
results = await remoteSearch(value, queryType, searchNodes) |
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.
Just a couple nitpicks :)
Can we have a race condition here? When the previous request returns after the current request and updates the items to stale results. I think it's possible with multiple HTTP/1.1 connections, but not sure about multiple streams on the same HTTP/2 connection, are they guaranteed to be ordered? Or maybe JS runtime resolves it in some way?
Also, do we need to debounce on remote search or check for response.ok
and results.length > 0
?
For some reason I decided to do these things in ruslandoga#1 but I don't remember if I actually had these problems or was just playing it safe ...
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. I'm sure you're right. As you can probably tell it's been almost a decade since I wrote any JavaScript so I'm still getting the hang of the new idioms.
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.
So looking at the code more carefully, it appears that the search
function is only called on page load, so should only be run once in the page's lifecycle.
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.
Additionally, the search result handlebars template takes care of whether any results were actually returned.
Okay so I've pushed some updates:
|
I'll be moving on to adding the search type dropdown in the search bar next. |
5ff3096
to
655d6af
Compare
Thank you @jimsynz. I have been a bit busy this week but I will try to add review and add polish to this feature this week. |
Thanks @josevalim 🙌 |
const index = await getIndex() | ||
|
||
// We cannot match on atoms :foo because that would be considered | ||
// a filter. So we escape all colons not preceded by a word. |
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.
Is this considered a filter by typesense?
end | ||
|
||
defp maybe_append_related_packages(search_nodes, _related_packages), do: search_nodes |
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.
We should revert these changes for now, I believe the related packages will come from hex.pm itself. You can emulate it though by calling mix docs
and then customizing the docs_config.js in doc yourself. :)
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.
Actually, we added our own for testing:
Line 127 in 8a5217e
File.write!("doc/docs_config.js", """ |
So I think you can just change the searchNodes
there, perhaps by adding Elixir itself or earmark_parser
to searchNodes, so you can test it too.
@@ -21,7 +21,7 @@ | |||
<link rel="canonical" href="<%= config.canonical %>" /> | |||
<% end %> | |||
<script defer src="<%= Assets.rev config.output, "dist/sidebar_items-*.js" %>"></script> | |||
<script defer src="docs_config.js"></script> | |||
<script defer src="<%= Assets.rev config.output, "dist/search_data-*.js" %>"></script> |
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.
This change should not be necessary either!
if (versionNodes.length > 0) { | ||
const latest = versionNodes[0] | ||
const searchNodes = getSearchNodes() | ||
const match = searchNodes.some(node => `v${node.version}` === latest.version) |
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.
I don't think we need to check if latest or not. It is easier if the user doesn't have to think about it. Regardless if it we are in the latest package or not, we should allow latest to be chosen (unless there is no searchNodes). So basically, we will have this:
- If searchNodes.length > 1, the related option is enabled, otherwise it is disabled
- If searchNodes.length > 0, the latest option is enabled, otherwise it is disabled
- Otherwise it is only current/local
If online, the default should be related > latest > current. If offline, it is the current one.
In other words, this should return return versionNodes.length > 0
.
Thank you @jimsynz, I have dropped some comments. About the online vs offline, maybe instead of checking it upfront, we should try to run the search and, if the search fails for any reason, we fallback to regular search with an error message? I believe there is a toast we could use. |
Thanks for the review @josevalim - I'll get onto it. |
Here's my first stab at adding latest and related search as per #2013
I'm keen for early feedback, thus this draft PR.
I have a few questions:
to_ast
is returns results forto ast
.ref
but I'd rather that the search index returned better results.I haven't yet changed any of the markup as I wanted to get this step right before I go adding things like package names/versions to results, etc.