feat: add DOM level 4 child element properties#652
feat: add DOM level 4 child element properties#652dmitri-gb wants to merge 1 commit intoxmldom:masterfrom
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. |
9cc3098 to
8a2671f
Compare
karfau
left a comment
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. |
Add `.children` property to Element, Document, and DocumentFragment using Object.defineProperty getters backed by LiveNodeList. Returns a live collection of direct Element children, filtering out text, comment, and other non-element nodes. Uses getter/setter approach with no extra state maintenance, aligning with maintainer preference expressed in PR xmldom#652 review. LiveNodeList handles cache invalidation automatically via _inc counter on DOM mutations.
Implements the [`ParentNode.children`](https://dom.spec.whatwg.org/#dom-parentnode-children) getter on `Element`, `Document`, and `DocumentFragment`. Returns a `LiveNodeList` of direct element children, filtering out text, comment, processing instruction, and other non-element nodes. ### Approach Uses `Object.defineProperty` getters backed by `LiveNodeList` — no extra state is maintained on the node. This follows the getter/setter approach [discussed in #652](#652 (comment)): > *"with the amount of state maintenance in this lib I'm leaning towards doing as many things as possible using a getter/setter approach"* The getter computes from `firstChild`/`nextSibling` on access. `LiveNodeList` handles cache invalidation automatically via the `_inc` counter on DOM mutations. ### Deviation from spec The DOM spec marks `.children` with [`[SameObject]`](https://webidl.spec.whatwg.org/#SameObject) (same reference on each access). This implementation returns a new `LiveNodeList` per access, matching the existing `getElementsByTagName` pattern. Caching per-node was avoided to keep zero extra state on nodes, consistent with the getter/setter preference above. Fixes #410
28aafda to
ea1b72c
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
7f17718 to
7699bab
Compare
This adds the properties childElementCount, firstElementChild, lastElementChild, nextElementSibling and previousElementSibling to Element. The first three are also added to Document and DocumentFragment.
7699bab to
444ada1
Compare
|
Hey, @karfau! Sorry for leaving this hanging for so long... I noticed that someone else recently implemented I also needed to remove the Additionally, I needed to update Jest to v30, as older versions are unable to serialize and send cyclic structures to Jest workers (the cyclic structures in this case are the DOM trees). This issue appeared after the inheritance-related change. I suspect that this may cause CI failures, as Jest v30 dropped support for Node.js v14 and v16. |
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.