ThemeQueryManager: Implement receive() #10014
Conversation
ockham
added
[Feature Group] Appearance & Themes
| import { DEFAULT_THEME_QUERY } from './constants'; | ||
|
|
||
| /** | ||
| * Constants | ||
| */ | ||
| const REGEXP_SERIALIZED_QUERY = /^(?:(\d+):)?(.*)$/; | ||
| const SEARCH_TAXONOMIES = [ 'subject', 'feature', 'color', 'style', 'column', 'layout' ]; |
There was a problem hiding this comment.
This is not the full set of filter taxonomies. It should probably be pulled from the object here if we really need to know about it. I'm worried about this though, because filtering should be completely transparent to the theme state.
There was a problem hiding this comment.
Wait! This is just for testing, right? Phew.
Can we move it into a test folder?
There was a problem hiding this comment.
Wait! This is just for testing, right? Phew.
No, not just for testing. This is used by isThemeMatchingQuery, which up until this PR would've been used as ThemeQueryManager's matches() method. Since we're dropping the latter now, only one use remains: client-side filtering of themes obtained from a Jetpack site's endpoint.
We do this here, and it essentially supersedes this stuff.
I'm worried about this though, because filtering should be completely transparent to the theme state.
I'd agree in principle, but as long as the JP endpoint differs from the WPcom one in this regard, I'd filter as early as possible after retrieval from the endpoint to be transparent afterwards.
I can pull the taxonomies from theme-filters. I think the themes list endpoint doesn't return them anyway (only the details one does), and the Jetpack one only returns feature (in the guise of tags, but we normalize that to feature).
Bottom line is that now that we don't use this for TQM, we might actually drop most of the taxonomies except for feature since they're not present in their remaining use case -- JP results filtering -- anyway.
| * algorithms on the client side would be non-trivial, or would require a | ||
| * heavy (and possibly slow) dependency such as elasticlunr. | ||
| * By implementing receive() ourselves instead, we can override the base class | ||
| * QueryManager's sorting behavior and keep the order as returned from the endpoint. |
There was a problem hiding this comment.
This comment is a bit verbose I think. We'd be crazy to implement some ES equivalent in the client, since we have a really fast ES endpoint that returns themes in the exact order we want them.
All we need to say is that the endpoint order is correct already. I think QueryManager exists because some data (posts?) needs to jump around? We don't have that problem, so we don't need to explain why we're not solving it.
There was a problem hiding this comment.
Hmm, if you inherit from QueryManager or PaginatedQueryManager, you're supposed to normally just implement matches and sort (as by the QM README) which is why I'm explaining why we don't do that.
But yeah, it is a bit verbose...
I tested theme upload as well, since that also uses ThemeQueryManager ;) |
Instead of just relying on that of the base class and implementing matches() and sort() to make use of it. This is because our themes query REST API endpoint uses ElasticSearch heavily to sort results by relevancy. Mimicking ES's complex weighting algorithms on the client side would be non-trivial, or would require a heavy (and possibly slow) dependency such as elasticlunr. By implementing receive() ourselves instead, we can override the base class QueryManager's sorting behavior and keep the order as returned from the endpoint.
It's no longer needed by ThemeQueryManager.
Jetpack sites don't return a `tier` field since all themes on a Jetpack site are free, so there's no point in filtering by tier.
ockham
force-pushed
the
add/theme-query-manager-receive-method
branch
from
51923ae to
ebc44e4
Compare
matticbot
removed
the
[Status] Needs Review
Instead of just relying on that of the base class and implementing
matches() and sort() to make use of it.
This is because our themes query REST API endpoint uses ElasticSearch
heavily to sort results by relevancy. Mimicking ES's complex weighting
algorithms on the client side would be non-trivial, or would require a
heavy (and possibly slow) dependency such as elasticlunr.
By implementing receive() ourselves instead, we can override the base class
QueryManager's sorting behavior and keep the order as returned from the endpoint.
Also, move
isThemeMatchingQuery()to state/themes/util since it's no longer needed byThemeQueryManager(Ironically, this moving around of code makes up the largest part of the PR.)To test: Check that theme sheets and the Current Theme bar still work. These are the only two places that already use
ThemeQueryManager.