Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix jsdom undefined nodeType for attributes #24

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix jsdom undefined nodeType for attributes #24

wants to merge 2 commits into from

Conversation

chiptus
Copy link

@chiptus chiptus commented Apr 26, 2017

I added a quickfix for cases where nodeType is undefined. it will check that the objects has the same constructor - meaning the same Class.

fix #22

It has changes in all files because I applied prettier on the files.

@coveralls
Copy link

coveralls commented Apr 26, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 92.308% when pulling bce38dd on chiptus:master into ac6eb6f on Olegas:master.

@coveralls
Copy link

coveralls commented Apr 26, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.5%) to 92.308% when pulling 63d8ded on chiptus:master into ac6eb6f on Olegas:master.

@Olegas
Copy link
Owner

Olegas commented Apr 27, 2017

@chiptus Let's remove last commit there are code style changes are made. It is not clean where is the main changes are.

@chiptus
Copy link
Author

chiptus commented Apr 30, 2017

reverted the changes and made the hotfix without style changes.

@coveralls
Copy link

coveralls commented Apr 30, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.1%) to 90.65% when pulling 3d86a79 on chiptus:master into 4fbf204 on Olegas:master.

@coveralls
Copy link

coveralls commented Apr 30, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-1.1%) to 90.65% when pulling 3d86a79 on chiptus:master into 4fbf204 on Olegas:master.

@Olegas
Copy link
Owner

Olegas commented May 1, 2017 •
edited
Loading

I don't think this fix is sufficient. What about this line: https://github.com/Olegas/dom-compare/blob/master/lib/compare.js#L104 ?

Here we need to know the correct node type to select proper comparison strategy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Comparing attributes gives 'nodeType is unexpected' error
3 participants
@chiptus @coveralls @Olegas