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.

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

Make Attr inherit from Node again #1641

Closed
brettz9 opened this issue Oct 29, 2016 · 7 comments · Fixed by #2770
Closed

Make Attr inherit from Node again #1641

brettz9 opened this issue Oct 29, 2016 · 7 comments · Fixed by #2770

Comments

@brettz9
Copy link

brettz9 commented Oct 29, 2016

The inheritance of Attr from Node was added back. Please support (although nodeName exists as an alias, nodeType is not available, etc.).

@snuggs
Copy link
Contributor

snuggs commented Dec 25, 2016

@domenic anyone working on this? If not can take a 🔪 at it.

@Robbert
Copy link

Robbert commented Jan 23, 2017

Last weekend I really needed Attr as Node again, and my attempted to do some refactoring turned out largely successful:
https://github.com/Robbert/jsdom/commits/issue-1641

I expect more work needs to be done to fully support Attr everywhere, in TreeWalker for example, I guess I should use annevk's commits to the DOM standard as checklist.

The main difficulty was the cyclic dependency of Node ⇒ attributes.js ⇒ Attr ⇒ Node, I noticed domenic ran into this issue earlier (#1247 (comment)). Since any type of Node usually is created through the Document, I chose to follow this pattern to remove the direct dependency of attibutes.js on Attr, and instead Attr is created through the ownerDocument of the ownerElement of the NamedNodeMap. This way no direct dependency on Attr is required.

Also, I'm registering all Node constructors on the Document implementation in a similar fashion as the custom element constructors for the HTML DOM are registered. See: Robbert@a62ec5d In another DOM implementation I worked on previously this approach worked splendidly.

If someone has time to give their opinion on this approach sometime, I can process the feedback and after that finish up details like TreeWalker support and updating the tests.

@snuggs
Copy link
Contributor

snuggs commented Jan 31, 2017

  1. @Robbert any way we can get this in a PR on your fork? Difficult to see the diffs.
  2. Can you rebase the branch to reflect recent changes? Also will help with visibility.
    Looking good but need to pull down branch to diff. I know there are ways in github but just too lazy. 😁 Still should do 1, and 2 tho.

Please advise.

@Robbert
Copy link

Robbert commented Jan 31, 2017

@snuggs Definitely, wasn't sure if it was appropriate to send a pull request for work in progress. Will do so this week.

@chiptus
Copy link

chiptus commented Apr 26, 2017

any update on this? is it still in progress? or should I try to take it?

@chiptus
Copy link

chiptus commented Apr 26, 2017

is there any other node that will return nodeType undefined?

@Robbert
Copy link

Robbert commented Apr 26, 2017

@snuggs I just sent the pull request for my branch, sorry it took so long. This week I have plenty of time to look into the failing unit tests, but I prefer to only spend more time on this when the approach I took is acceptable.

domenic added a commit that referenced this issue Apr 16, 2018
This includes some un-tested and inactive code for Attr nodes, in preparation for fixing #1641.
domenic added a commit that referenced this issue Apr 23, 2018
This includes some un-tested and inactive code for Attr nodes, in preparation for fixing #1641.
domenic added a commit that referenced this issue Apr 23, 2018
This includes some un-tested and inactive code for Attr nodes, in preparation for fixing #1641.
domenic pushed a commit that referenced this issue Jan 25, 2020
Fixes #1641. Closes #1822 by superseding it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants