fix: restore more Node and ProcessingInstruction types #726
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #726 +/- ##
=======================================
Coverage 94.36% 94.36%
=======================================
Files 8 8
Lines 2093 2094 +1
Branches 537 538 +1
=======================================
+ Hits 1975 1976 +1
Misses 118 118 ☔ View full report in Codecov by Sentry. |
| assert(fakeNode.nodeType, undefined); | ||
| assert(fakeNode.lineNumber, undefined); | ||
| assert(fakeNode.columnNumber, undefined); | ||
| assert(fakeNode.textContent, undefined); |
There was a problem hiding this comment.
This doesn't really make sense since no matter what property you try to access it's always gonna be undefined since fakeNode is just an empty object. You should probably just use an Element or something else that extends Node.
There was a problem hiding this comment.
As I mentioned in the comment above this section, yes, there is no actual instance of type Node, but these assertions should make sure that those properties are available on the type Node, not only on some sub type/class.
When using assert, the CodeQL checks will complain that the values are unused.
And yes, I'm asserting that the value is undefined, which only makes sense in the context of this assertion.
There was a problem hiding this comment.
Ahh I think I get it now... so basically if you would try to do this: assert(fakeNode.doesntExist, undefined) JS would be fine with it but since the TS compiler checks this the check would fail correctly.
|
Btw other than that it looks good to me and can be released. #728 is building on this branch ;) |
|
@Ponynjaa thx for the review, I integrated your suggestions. |
and avoid redundant uncheckable conditions
Node.nodeTypeNode.textContentNode.columnNumberNode.lineNumberProcessingInstruction.data(inherited fromCharacterData)ProcessingInstruction.targetfixes #725