Simplify Nuxt query parameter computation #3136
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
obulat
added
🟧 priority: high
|
Size Change: -609 B (0%) Total Size: 938 kB
ℹ️ View Unchanged
|
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
from
2f8d0ab to
3db4968
Compare
8 tasks
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
3 times, most recently
from
351b8c8 to
eddeee5
Compare
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
from
eddeee5 to
57f6449
Compare
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
2 times, most recently
from
6094566 to
f3320ca
Compare
|
Based on the high urgency of this PR, the following reviewers are being gently reminded to review this PR: @AetherUnbound Excluding weekend1 days, this PR was ready for review 11 day(s) ago. PRs labelled with high urgency are expected to be reviewed within 2 weekday(s)2. @obulat, if this PR is not ready for a review, please draft it to prevent reviewers from getting further unnecessary pings. Footnotes
|
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
from
f3320ca to
e6eb238
Compare
sarayourfriend
approved these changes
Oct 22, 2023
frontend/src/stores/search.ts
Outdated
Comment on lines
78
to
82
| if (!isSearchTypeSupported(searchType)) { | ||
| return { q: searchTerm.trim() } | ||
| } |
There was a problem hiding this comment.
What are the circumstances in which this would be the case? I'm not fully remembering how the supported/non-support search type thing works or why the code needs to handle both cases when only one is actually used (supported).
There was a problem hiding this comment.
If you turn on additional_search_types feature flag and look into the content switcher in the search results page header, the links to the types that are "Coming soon" will have the q=cat (or whatever) parameter set instead of just linking to /search/video (which will probably just redirect to the homepage since we don't have a generic media type page without a search term).
frontend/src/stores/search.ts
Outdated
| if (query[api_param_name]?.length) { | ||
| search_query[api_param_name] = query[api_param_name] | ||
| } | ||
| const search_query: SearchQuery = { |
There was a problem hiding this comment.
Can we change this to use camel case, while we're modifying this function anyway?
Suggested change
| const search_query: SearchQuery = { | |
| const searchQuery: SearchQuery = { |
frontend/src/stores/media/index.ts
Outdated
Comment on lines
446
to
447
| const queryParams = useSearchStore() | ||
| .searchQueryParams as PaginatedSearchQuery |
There was a problem hiding this comment.
Why is the case necessary here? Could we move it into the search store's searchQueryParams getter? Is there some other way to refine this type that wouldn't require a cast?
There was a problem hiding this comment.
search store can compute all of the parameters except for the page, which is saved in the media store.
I was trying not adding the page parameter to the queryParams when sending the first search request, that's why I wrote this code like this.
I refactored it now to remove the type cast.
frontend/src/stores/search.ts
Outdated
Comment on lines
220
to
226
| ? this.frontendSearchUrlParams | ||
| : computeQueryParams( | ||
| searchType, | ||
| this.filters, | ||
| this.searchTerm, | ||
| "frontend" | ||
| ) |
There was a problem hiding this comment.
What's the difference between using frontendSearchUrlParams and calling computeQueryParams with the frontend strategy? frontendSearchUrlParams's implementation looks the same as the else branch of this ternary. Is the difference in passing searchType directly vs using this.searchType as frontendSearchUrlParams does? In what circumstances would type be undefined? Can those circumstances safely fallback to ALL_MEDIA, maybe?
If anything, a comment here would be helpful to explain why the branches are different and why an undefined search type dictates which we choose. Whatever the distinction is between these two branches seems to be very subtle and is completely lost on me.
There was a problem hiding this comment.
I simplified this code. The type is passed here when creating a search page link (in the content switcher or in the content link on the All content search page) for a type that's different from the current search type.
| @@ -508,7 +485,7 @@ export const useSearchStore = defineStore("search", { | |||
| filterCategory: FilterCategory | |||
| ): boolean | undefined { | |||
| if (!["licenseTypes", "licenses"].includes(filterCategory)) { | |||
| return | |||
| return false | |||
There was a problem hiding this comment.
Does this change allow us to remove undefined from the return type of this function on line 486?
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
from
6da4e5f to
033cc95
Compare
obulat
force-pushed
the
additional_search_views/store-remove-searchby
branch
from
033cc95 to
e7a091e
Compare
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.
Fixes
Related to #2858 by @obulat
Note: Most of the added lines are the saved API responses (tapes)
Description
This PR simplifies one of the most complicated parts of the Nuxt application: the building of API query string from the query parameters, and parsing of the query string into the filter parameters. This will make adding the store changes for Additional search views project much easier to do.
searchByfilter kind was removed because it does not allow for the flexibility required by the Additional search views project. It will be replaced by a separate parameter on the search store state in the future PR.prepare-search-query-paramsmodule was completely removed because it is no longer needed. Its functions are implemented more flexibly incomputeQueryParamsfunction.Some unused functions were removed. For example, we no longer parse the query string into filters, and use the
route.paramsinstead.This PR also adds more concise types for the search query.
Testing Instructions
The querying should work as expected, and CI should pass.
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin