Simplify ViewResolver type
Created by: lguychard
A ViewResolver
is now simply a function that takes a container as input and returns Iterable<View>
, instead of being defined with a selector and a resolveView()
function.
Merge request reports
Activity
Created by: lguychard
When we wrote
trackViews()
, we opted not to run the resolvers on removed nodes, to guard against cases where resolution depends on state outside the code view, and the needed state would be inaccessible because the node has been removed from the tree. It wasn't a bug we actually encountered, merely something that was raised during code review, and then accounted for, but it is definitely possible. The result would have been leaked code views. Reference: !2909 (merged)The issue you raised in !3920 (merged) is a different bug, that leads to systematically leaked code views: if the resolver returns a different element than the selector, the code view will be leaked when removed from the DOM, because the resolver is not run on removed nodes.
This PR does reintroduce the first bug, in order to fix the second.
I do think that looking at the context of an element initially matched with a selector in order to return the correct code view element is a valid use case. On github.com, for instance, we can't come up with an unambiguous enough selector to match the whole code view on blob views (example), and the issue was the same for snippets.
One way to keep this possible while avoiding both bugs would be to simply check whether removed nodes are tracked views, or contain tracked views within their subtree (without running selectors or resolvers on removed nodes).
Created by: lguychard
Your suggestion is still potentially vulnerable to !2909 (merged) if
findElements()
does something akin to this code, isn't it?