♻️ Remove broken reference to this inside static class method#29695
♻️ Remove broken reference to this inside static class method#29695kristoferbaxter wants to merge 1 commit intoampproject:masterfrom
Conversation
|
Hey @Jiaming-X, @jeffkaufman! These files were changed: |
| */ | ||
| static createIfResponsive(element) { | ||
| if ( | ||
| !this.isContainerWidth_ && |
There was a problem hiding this comment.
While this PR doesn't change behavior, do we know what the original intent was?
There was a problem hiding this comment.
I think this should be changed to an instance method, and the intent is to check this.isContainerWidth_ on the instance.
There was a problem hiding this comment.
My take is the intent is not clear.
I read this as a constructor replacement either returning null or a new class instance based on some conditions. One of these conditions is not possible.
There was a problem hiding this comment.
Additionally, refactoring this to no longer be static is significantly altering it and callers usage.
When @Jiaming-X has a chance to review, it would be good to learn what they prefer.
There was a problem hiding this comment.
Lastly, there is no test covering this usage.
If there is an intent for this code to branch the logic within the static method, lets also add a test covering it.
There was a problem hiding this comment.
+1, intent isn't clear here. I'm fine with submitting this as a fix to unblock esm/strict modes. But we should also follow up to figure out what intent was.
| */ | ||
| static createIfResponsive(element) { | ||
| if ( | ||
| !this.isContainerWidth_ && |
There was a problem hiding this comment.
I think this should be changed to an instance method, and the intent is to check this.isContainerWidth_ on the instance.
|
The only times this static method is called are... This is within the
Overall I find the code a bit confusing to read, but perhaps someone more knowledgeable of its intended use can tell us what the original intent was since its impossible for a |
samouri
left a comment
There was a problem hiding this comment.
Approving with the caveat that we should also follow up to make sure its WAI.
|
My reading is that |
|
I think there are two ways the ad can be upgraded. One is to full viewport width, and the other (for desktop) is to container width. I think that this check was intended to not upgrade to full width if it was already determined to be container width, but the way it is written I don't think the check is actually doing anything. Jiaming should be able to confirm tomorrow (London time zone). |
|
Hey everyone, just to provide more context here: The original flow of Ad Size Optimization used To fix these issues, I added one more subflow For To move forward, the sub flow convertToContainerWidth_ might need different implementation. To unblock you from the new build, we could disable to flow first and remove the this.isContainerWidth_ usage from static methods. I can change the implementation later in a separate thread. For my case, it will impact 0.3% of the traffic. https://screenshot.googleplex.com/OAtWpn9fvpt WDYT? Related links: |
|
I'm going to close this issue and defer to @Jiaming-X's PR. Thanks for the input everyone! As a followup, we should invest in better test coverage of this space so refactoring can begin. |
This reference doesn't exist within a static class.