-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update jsdom to v16 #20
Conversation
Pull Request Test Coverage Report for Build 116
💛 - Coveralls |
5dc88ae
to
3f41c8d
Compare
3f41c8d
to
9bafe60
Compare
9bafe60
to
3a5d9f4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests are all passing and the changes here look pretty straightforward. One little note, but otherwise good
@@ -625,6 +625,10 @@ exports.onerror = null; | |||
* @return {Node} DOM Node | |||
*/ | |||
var patch = exports.patch = function(elem, jml, filter) { | |||
// Cannot patch text nodes | |||
if (elem.nodeType === 3) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is all over the codebase, but it might be nice to use the constant here (Node.TEXT_NODE
) so you don't need the comment. I understand if you want to leave it so it's consistent though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
100% agree. I think constants would be a good refactor. went for consistency
Used in the DOM interface for using JsonML to interact with the DOM and impacts functionality within a node runtime environment. Update the html.patch method to prevent raising an error on text nodes. Prior to updating jsdom, the node was not updated but no error was emitted.
3a5d9f4
to
460a728
Compare
Description
Used in the DOM interface for using JsonML to interact with the DOM
and impacts functionality within a node runtime environment.
Update the html.patch method to prevent raising an error on text nodes.
Prior to updating jsdom, the node was not updated but no error was emitted.
Types of changes
Related Issue
n/a
Motivation and Context
Keeping dependencies up to date.
How Has This Been Tested?
This should be pretty well covered by existing tests. I worked through and extended coverage in #25 and #26 and feel pretty good about this.
Screenshots (if appropriate):
n/a
Checklist: