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
remove cached owner info #6028
remove cached owner info #6028
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -103,6 +103,15 @@ export class Resource { | |
Resource.forElementOptional(element).updateOwner(owner); | ||
} | ||
element[OWNER_PROP_] = owner; | ||
|
||
// Need to clear owner cache for all child elements | ||
const cachedElements = element.getElementsByClassName('-amp-element'); | ||
for (let i = 0; i < cachedElements.length; i++) { | ||
const ele = cachedElements[i]; | ||
if (Resource.forElementOptional(ele)) { | ||
Resource.forElementOptional(ele).updateOwner(undefined); | ||
} | ||
} | ||
} | ||
|
||
/** | ||
|
@@ -190,7 +199,7 @@ export class Resource { | |
|
||
/** | ||
* Update owner element | ||
* @param {!AmpElement} owner | ||
* @param {AmpElement|undefined} owner | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How would this be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. when I want to clear the cached value and reset it to undefine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, I didn't see that. |
||
*/ | ||
updateOwner(owner) { | ||
this.owner_ = owner; | ||
|
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.
We definitely want this to be recursive, as it could be
<owner> <child> <div> <grandchild>
. UsingResources#discoverResourcesForElement_
or#discoverResourcesForArray_
would do it.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.
Good point, it needs to be recursive. However I don't think
Resources#discoverResourcesForElement_
or#discoverResourcesForArray_
will do the work here. In the case of<owner> <child> <div> <grandchild>
#discoverResourcesForElement_
will stop searching when it reaches<child>
. Thus will never clear the cached value for<grandchild>
. Also what if<owner><child><div><grandchild><grandgrandchild>
?A correct approach I can think of would be finding all the children, grandchildren .... (sorry I'm too lazy find the opposite word for ancestor 😄), but I assume it would be time consuming.
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 switched to
getElementByClassName('-amp-element')
which finds me all the amp children.It's same concept as
discoverResourcesForElement_
exceptdiscoverResourcesForElement
doesn't work in case<owner><child><grandchild><greatgrandchild>
because it will not clear cache for greatgrandchild.PTAL