feat: add DOM level 4 child element properties #652
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #652 +/- ##
==========================================
+ Coverage 95.05% 95.11% +0.05%
==========================================
Files 8 8
Lines 2165 2191 +26
Branches 569 576 +7
==========================================
+ Hits 2058 2084 +26
Misses 107 107 ☔ View full report in Codecov by Sentry. |
This adds the properties childElementCount, firstElementChild, lastElementChild, nextElementSibling and previousElementSibling to Element. The first three are also added to Document and DocumentFragment.
9cc3098 to
8a2671f
Compare
There was a problem hiding this comment.
Thx for picking it up and investing into the tests. Did you read all the comments in the previous PR?
I think there was an idea to implement these functions in a "getter/setter" fashion that operates on the childNodes to avoid having to maintain redundant state... But maybe you already considered that, not sure, will need more time to understand the changes.
| test('appendElement and removeElement', () => { | ||
| const dom = new DOMParser().parseFromString(`<root><A/><B/><C/></root>`, MIME_TYPE.XML_TEXT); | ||
| test('appendChild and removeChild', () => { | ||
| const dom = new DOMParser().parseFromString(`<root><A/>x<B/>y<C/></root>`, MIME_TYPE.XML_TEXT); |
There was a problem hiding this comment.
I might need to check out the code to understand this change. Could we keep the existing test and add a new one for the new cases?
Or does it cover all 4 funtions, in that case Iwould prefer the message to cobtain all of them.
There was a problem hiding this comment.
It only covers the two functions -- appendChild and removeChild. The test simply had the names wrong.
| expect(doc.type).toBe('xml'); | ||
| }); | ||
|
|
||
| test('should create a Document with root element in a named namespace', () => { |
There was a problem hiding this comment.
It was an exact duplicate of the test above.
I didn't spot any such idea in the previous PR. I'm not sure if that's really the way to go though. Accessing something as simple as |
It's always a trade between either keeping more state (in memory) and correctly updating all references when modifying the DOM (which also takes time and those things are happeing when ever a node is added during parsing), even when there might never be any access to that state, vs accessing the internal state when needed and have a (slightly) longer runtime for accessing those values. with the amount of state maintenance in this lib I'm leaning towards doing as many things as possible using a getter/setter approach, but it has not been applied widely yet, and there might be issues with how "prototype inheritence" is implemented in this lib, which could get into the way of doing it like this, if it is implemented on prototypes that are copied. Just thinking out loud here. I would love to be more active on this repo, but this is only very likely to happen at the end of the year or start of the next year. |
Hey!
Since #398 seems to be stale and apparently no longer being worked on by the original author, I figured I'd make another attempt.
This PR adds the following properties to
Elementnodes:childElementCountfirstElementChildlastElementChildnextElementSiblingpreviousElementSiblingThese properties correspond to the attributes defined in the
ParentNodeandNonDocumentTypeChildNodemixins in the DOM Living Standard.The first three of these properties are also added to
DocumentandDocumentFragment.