Don't overwrite children without ID when appending/prepending #285
Conversation
|
The CI failure looks like an unrelated (flaky?) test |
|
Yeah, that test is super flaky. Would be nice to have someone investigate that as well. Thanks for the quick fix! I'll get another beta release out in not too long. |
| @@ -36,7 +36,7 @@ export class StreamElement extends HTMLElement { | |||
| } | |||
|
|
|||
| get duplicateChildren() { | |||
| return [...this.templateContent?.children].map(templateChild => { | |||
| return [...this.templateContent?.children].filter(templateChild => !!templateChild.id).map(templateChild => { | |||
| let targetChild = [...this.targetElement!.children].filter(c => c.id === templateChild.id)[0] | |||
| return { targetChild , templateChild } | |||
There was a problem hiding this comment.
I'd like to suggest a slight adjustment. Since the method is "find duplicate children in the TARGET with the same ID as the TEMPLATE", I think the code should filter the TARGET children with id's.
get duplicateChildren() {<br class="Apple-interchange-newline">
return [...this.templateContent?.children].map(templateChild => {
- let targetChild = [...this.targetElement!.children].filter(c => c.id === templateChild.id)[0]
+ let targetChild = [...this.targetElement!.children].filter(c => c.hasAttribute('id') && c.id === templateChild.id)[0]
return { targetChild , templateChild }It's also a bit more efficient (less loops).
There was a problem hiding this comment.
I think the code should filter the TARGET children with id's.
It's also a bit more efficient (less loops)
Either I don't understand your comment, or I simply don't agree (or both 😄)
the bit I added: .filter(templateChild => !!templateChild.id) runs before the map, so it ensures that the inner loop is only executed for the ids that exist in the template. So there are less loops in my version than in the one you suggest, no ?
In my code, the only way c.id === templateChild.id in the inner filter is that c.id is not empty (since the filter ensures that templateChild.id is not empty), so the added test c.hasAttribute('id') is redundant.
Am I missing something ?
There was a problem hiding this comment.
Clarification: "less loops" assuming the this.targetElement has more children than this. templateContent. Otherwise, if the inverse I can see why we might filter by target.
One additional efficiency I noticed the code generates [...this.targetElement!.children] array for every templateChild in the loop. It could be created once before the looping over this.templateContent?.children.
There was a problem hiding this comment.
I recognize the redundancy of both changes. I was suggesting filtering the targetChildren without ids instead of filtering out the templateChildren without ids. I was not suggesting we do both. They both accomplish the same result, it was feedback. based on code comprehension/readability and not functionality.
FYI, I started my code review prior to being merged.
There was a problem hiding this comment.
Was just working on this code in the branch to add css selectors for multiple replaces, and came up with this that I found much easier to follow/read:
removeDuplicateTargetChildren() {
this.duplicateChildren.forEach(c => c.remove())
}
get duplicateChildren() {
const existingChildren = this.targetElements.flatMap(e => [...e.children]).filter(c => !!c.id)
const newChildrenIds = [...this.templateContent?.children].filter(c => !!c.id).map(c => c.id)
return existingChildren.filter(c => newChildrenIds.includes(c.id))
}Have not considered performance ramifications, though.
There was a problem hiding this comment.
I agree it's easier to follow.
I'm not clear on this.targetElements.flatMap vs the prior this.targetElement, but if using this solution with this.targetElement it's generally faster where there are more than a couple of elements in either the template or target. If just one, it's slightly slower (but neglable).
There was a problem hiding this comment.
The targetElements is from the PR that allows CSS selectors for targets.
There was a problem hiding this comment.
I like the suggested changes.
| @@ -8,14 +8,22 @@ export class StreamElementTests extends DOMTestCase { | |||
| } | |||
|
|
|||
| async "test action=append"() { | |||
| const element = createStreamElement("append", "hello", createTemplateElement(" Streams")) | |||
| const element = createStreamElement("append", "hello", createTemplateElement("<span> Streams</span>")) | |||
| const element2 = createStreamElement("append", "hello", createTemplateElement("<span> and more</span>")) | |||
There was a problem hiding this comment.
Would we want to make this more clear what we are testing?
"<span> Element without ID</span>"
"<span> another element without ID</span>"
There was a problem hiding this comment.
Maybe, although since the goal of this change is to catch regressions, I'm not sure it's that important in the long run. (i.e. if the code is broken at some point in the future this test will fail and it will be obvious why).
I would have updated the code, but the PR is now merged, so if you feel strongly about it feel free to open another PR for it.
There was a problem hiding this comment.
Again, opinionated feedback based on future code comprehension and readability. If a year from now a developer is trying to understand "Why are we testing adding span elements without ID's, seems redundant. We could simplify this test and only check if one item is added". Adding the additional information provides clarity for future developers to understand that we are testing elements without ids.
Fixes #284
cc: @dhh @pbstriker38
This was indeed a mistake in #240 so I updated the append/prepend tests to use
<span>elements rather than plain text to reproduce the problem and added the fix for it.