티스토리 수익 글 보기
Clarify docs around a tag visitor’s boolean return value #1479
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you’re merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project’s expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
felixarntz
left a comment
There was a problem hiding this comment.
@westonruter LGTM!
Given the clearer terminology of “track” vs “visit”, should we consider renaming relevant code bits as well? Even if any of the API pieces within Optimization Detective use that, we could probably rename, assuming that so far it’s only ourselves that have built integrations with it and we haven’t documented it much anyway? Alternatively, we could deprecate the old ones to be super safe.
|
@felixarntz which code bits are you referring to? |
| * @return false Whether the tag should be recorded in URL metrics. | ||
| * @return false Whether the tag should be tracked in URL metrics. | ||
| */ | ||
| function auto_sizes_visit_tag( OD_Tag_Visitor_Context $context ): bool { |
There was a problem hiding this comment.
which code bits are you referring to?
Haven’t looked at the codebase outside this PR, but this function for example would better have a different name now. Of course this is just a plugin consuming Optimization Detective, but it led me to bring this up.
But there’s definitely also relevant code in Optimization Detective itself, e.g. the od_register_tag_visitors action could probably use a more accurate name too.
There was a problem hiding this comment.
These are still tag visitors though. That doesn’t change here. What is changed is that tag visitors always visit tags (and they always have) but what is made more clear is what the return value means from a tag visitor. A true return value means that it is indicating that the tag should be tracked/registered/monitored in URL metrics. In the case of the Enhanced Responsive Images plugin, the tag visitor never needs to read the tag out of URL metrics so it always returns false.
There was a problem hiding this comment.
But why are they called “tag visitors”? I agree with the rationale for the changes in this PR, but in the same sense I find it equally unclear what “visiting” is about when looking at the API as a whole. “Tracking” a tag makes a lot more sense for me in describing the purpose of why I should be using this API. The purpose, as far as I understand, is to track certain tags – which implies “visiting”, but that’s not really the purpose and less clear to me.
There was a problem hiding this comment.
Tag visitors visit every open tag in the document.
Only a subset of tag visitors need to track the tag in URL metrics. If a tag visitor doesn’t need URL metrics to perform it’s optimizations (as is the case for auto-sizes) then no tracking is happening.
|
To merge this we first need #1478 to be reviewed and merged 🙏 |
Important
This is a follow-up PR to #1478. Merge that PR into
trunkbefore merging this PR, since it is branched off of that PR’s branch.Fixes #1342
The terminology around a tag visitor “visiting” a tag is fuzzy. It’s not clear what it means for a tag to be “visited”. In reality, what this means is that the tag is tracked/recorded in URL metrics so that it appears among its
elements, including theintersectionRectandclientBoundingRectamong other aspects captured bydetect.js. If a tag visitor does not need any of that information, then it can just always returnfalse, even though it may have “visited” a tag by applying some optimization to it. For example. Enhanced Responsive Images (auto-sizes) has a tag visitor doesn’t need that information to apply its optimizations as it just addsautoto thesizesattribute if it is lazy-loaded and doesn’t haveautoalready. This is a case where the tag visitor always returnsfalse.As explained in #1342, instead of returning a boolean value for whether to track the tag in URL metrics, it could be more sophisticated by detecting whether the tag visitor tried to access information about that tag from the stored URL metrics. Or instead of returning a boolean there could be a method on the context object like
$context->track_in_url_metrics()which would do the same. I could go either way. But these changes in the PR are a good iterative improvement at least and we can add such a method later.