Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #728 +/- ##
==========================================
+ Coverage 94.36% 94.78% +0.41%
==========================================
Files 8 8
Lines 2094 2165 +71
Branches 538 569 +31
==========================================
+ Hits 1976 2052 +76
+ Misses 118 113 -5 ☔ View full report in Codecov by Sentry. |
|
FYI: I'm not done yet. I only added the logic and some descriptions to the props/methods that I added however I didn't have time yet to update the README, add typings or add tests. |
karfau
left a comment
There was a problem hiding this comment.
I just had a first glance at the diff, but already wanted to share the feedback
|
|
I don't think so. One possible approach would be to vendor the whole But I think we will need some kind of helper to make it possible/easy to tell typescript it should be treated as compatible. Maybe by using some generic types? |
lib/dom.js
Outdated
| if (!other) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I would suggest we add a test that passes undefined or null to the function to cover the condition.
It makes sense to have it, even though the types say you can only pass a .Node which can never be falsy
lib/dom.js
Outdated
| (this.childNodes && otherNode.childNodes && this.childNodes.length !== otherNode.childNodes.length) || | ||
| (!this.childNodes && otherNode.childNodes) || | ||
| (this.childNodes && !otherNode.childNodes) |
There was a problem hiding this comment.
All Nodes have a childNodes attribute, right? How can it not be set?
I think it is because we do not really have classes and inheritance so the property is not set from all the "constructors"...
We could fix it in this PR, by setting childNodes in all contructors, with the benefit of simpifying this check to
this.childNodes.length !== otherNode.childNodes.length,
or we keep it for another PR and will file an issue.
What do you think?
There was a problem hiding this comment.
FYI: I added childNodes = new NodeList() for every Document.create*() => Node method, but kept the condition for now, we will see if it's possible to produce test cases for all the missing cases.
karfau
left a comment
There was a problem hiding this comment.
Completed my review, once the comments have been resolved, I will approve it.
|
By the way, collaboration would be much easier for me if you push the branch to the xmldom repository, which you should be able to. Update: I have now configured your repo as a remote, so we can keep this option for the next PR and continue on this one in your repo. |
# Conflicts: # examples/typescript-node-es6/src/index.ts # index.d.ts # lib/dom.js
|
I resolved most of my comments, but there are some uncovered cases, which we need to add tests for. |
|
I agree, we should just remove the checks for Btw I have no idea why CodeCov wanted tests for this added line but it seems like it doesn't want them anymore soooo.. works for me ^^ |
and decouple tests from each other
Because there are no tests for Even though codecov seems to be happy (I need to see how to configure this better), I would prefer that all changed code is covered before the PR is merged. |
and mark it as deprecated, since it is since DOM Level 4
karfau
left a comment
There was a problem hiding this comment.
Thx, great contribution and collaboration
Adds missing properties and methods of Node to be compatible with the default DOM-typings.
Added properties
parentElement,baseURI,isConnected.Added methods
contains(otherNode),getRootNode(options),isEqualNode(otherNode),isSameNode(otherNode).