Restore type-directed search #1325
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fsoikin
commented
Feb 9, 2025
fsoikin
commented
Feb 9, 2025
| @@ -102,9 +113,8 @@ query | |||
| :: TypeIndex | |||
| -> TypeQuery | |||
| -> Aff { index :: TypeIndex, results :: Array SearchResult } | |||
| query typeIndex typeQuery = do | |||
| res <- lookup (stringifyShape $ shapeOfTypeQuery typeQuery) typeIndex | |||
| pure $ res { results = res.results } | |||
fsoikin
commented
Feb 9, 2025
Comment on lines
+73
to
+74
| , log :: String -> Aff Unit | ||
| , die :: String -> Aff Unit |
There was a problem hiding this comment.
IndexBuilder was logging directly to console with no indirection at all. I added these parameters so that it would respect the --quiet option.
Looking at the larger picture, I don't understand why this is a separate project and not part of Spago proper. It's not used from anywhere else, only from the docs command implementation.
There was a problem hiding this comment.
It was born as a separate experiment, and was integrated only recently in Spago's codebase. It could be used independently from the command line as well (functionality that we ought to restore as a Spago command)
fsoikin
commented
Feb 9, 2025
fsoikin
commented
Feb 9, 2025
Comment on lines
-236
to
+232
| (mkHeader typeShape <> JSON.print (CJ.encode codec results)) | ||
| (mkHeader typeShape <> JSON.print (CJ.encode codec $ fromMaybe [] results)) | ||
| where | ||
| mkHeader typeShape = | ||
| "// This file was generated by docs-search\n" | ||
| <> "window.DocsSearchTypeIndex[\"" | ||
| <> typeShape | ||
| <> "\"] = " | ||
|
|
||
| codec = CJ.Common.maybe $ CJ.array SearchResult.searchResultCodec | ||
| codec = CJ.array SearchResult.searchResultCodec |
There was a problem hiding this comment.
These lines are the actual fix of the actual problem.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description of the change
Fixes #1093
The problem was that
IndexBuilderwas writing aMaybevalue into index files, and it came out like{ tag: "Just", value: [ ... ] }, while the UI was expecting to read anArrayvalue, not wrapped in aMaybe, silently failing at that, and returning empty results. I suspect the bug was introduced during migration from Argonaut to Codec, because they have differentMaybesemantics: Argonaut writes the value itself forJustornullforNothing, while Codec apparently wraps it like any other ADT.To fix, I made the
IndexBuilderwrite a nakedArrayvalue, coercingNothingto an empty array.I took the opportunity to also clean up and straighten the code a bit. See comments in the code.
I did not add a test for this particular issue, because the test would have to be an integration one: build docs, open browser under Selenium, try to search. I think it would be valuable to add such test, but it felt like too big of a scope for this fix.
Checklist:
[ ] Added some example of the new feature to theREADME[ ] Added a test for the contribution (if applicable)