-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
HTML search: serialize search index as JSON #13097
Closed
jayaddison
wants to merge
31
commits into
sphinx-doc:master
from
jayaddison:issue-13096/html-search-handle-proto-query
Closed
Changes from 5 commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
47f6547
search: check that query terms exist as properties in term indices be…
jayaddison 713d6bb
Tests: fixtures: add document title for ECMAScript testroot
jayaddison 0fff170
Tests: add JavaScript test coverage for prototype-property-name query
jayaddison 64d6e09
Add CHANGES.rst entry
jayaddison 9bab17c
Tests: add negative-test case
jayaddison 9b1edac
search: add explanatory comment for document title/text term lookup
jayaddison 52b9de3
search: retain `undefined` result instead of `[]` for non-matching terms
jayaddison bcb02fe
search: add explanatory comment regarding `hasOwnProperty` usage
jayaddison 7267990
Merge branch 'master' into issue-13096/html-search-handle-proto-query
AA-Turner 462c859
HTML search: encapsulate the search index content within a JSON string
jayaddison 443fd21
HTML search: use the `JSON.parse` `reviver` argument to freeze search…
jayaddison 295d230
Merge branch 'master' into issue-13096/html-search-handle-proto-query
jayaddison 3498755
Tests: fixup: accommodate adjusted `setIndex` prefix/suffix lengths
jayaddison f38ce20
HTML search: replace `repr` with simple embedded single-quotes
jayaddison 0d9a26c
Revert "HTML search: use the `JSON.parse` `reviver` argument to freez…
jayaddison d4bba1e
WIP: HTML search: use JS `Map`s in serialized index
jayaddison d9dee52
HTML search: fixup for object search
jayaddison d056a8a
HTML search: ensure array-format index entries are sorted
jayaddison 15a7dfa
HTML search: refactor-out redundant code
jayaddison 6e670ea
HTML search: rectify variable names
jayaddison f2848ed
HTML search: compress index representation
jayaddison 798cb48
HTML search: enclose index names in double-quotes
jayaddison af61293
Tests: HTML search: use `ast.literal_eval` to parse index contents
jayaddison 2bcf7a6
Tests: HTML search: update test expectation
jayaddison 381c26f
HTML search: cleanup: remove redundant `assert` statement
jayaddison ac281dc
HTML search: typing/lint fixups for `ruff` and `mypy`
jayaddison fae81a0
Edit CHANGES.rst entry
jayaddison 6d5a121
HTML search: use JSON format for the search index
jayaddison 39b4a4e
Merge branch 'master' into issue-13096/html-search-handle-proto-query
jayaddison 7428932
Tests: cleanup: remove unused `ast` import
jayaddison 22acc49
HTML search: refactor loader to use Fetch API
jayaddison File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
Empty file.
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
ECMAScript | ||
---------- | ||
|
||
This is a sample JavaScript (aka ``ECMAScript``) project used to generate a search engine index fixture. | ||
|
||
.. js:attribute:: Object.__proto__ | ||
jayaddison marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Used to access the `prototype <https://developer.mozilla.org/en-US/docs/Learn/JavaScript/Objects/Object_prototypes>`_ of an object instance. |
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
Oops, something went wrong.
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.
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'm actually a little confused by how this might work, as it doesn't appear that JavaScript supports setting the
__proto__
property in an object. Am I missing something? Should we be using a map to store these terms instead?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.
When parsing JSON, an object key named
"__proto__"
is accepted, and converted to the JavaScript object key["__proto__"]
-- distinct from the__proto__
property (a reference to a class prototype).During read-access, introducing
hasOwnProperty
allows us to filter out the latter, so that we don't mistakenly retrieve the prototype class reference fromterms
.I'm not certain whether the below example will help to clarify, but I find experimentation helpful to understand some of the behaviours:
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 forgot to respond to your
Map
question: that is certainly possible, although I feel that doing so might add some runtime overhead (since we'd need to load the native JS object contents from the index into respectiveMap
objects).As I understand it,
Map
objects are also more difficult to make immutable in JavaScript thanObject
andArray
instances. Freezing aMap
object doesn't prevent its private contents from being updated:So another factor is that, to the extent that I'd like to see the search index contents become immutable at load-time in future (#13098), I'd prefer not to migrate to the
Map
type unless there is a compelling reason to (maybe avoiding prototype pollution is that reason -- but I don't think we're exposed to that currently).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.
Ok got it, this makes sense to me re: the property. TIL!
I'm a little bit less sure about whether the index should be immutable, but happy to defer that discussion to another day.
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, one extra piece of optional feedback: since this is kind of counter-intuitive, you might want to add a comment on this line explaining what you're doing.
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.
Yep, OK - I'll try to add some phrasing to explain this succinctly. Thanks for the code review!