Looks like there’s some weird circular dependency issues.

One way to fix it would be to change WebIDL2JS to generate exports.* = assignments, rather than module.exports = iface;.

Oh, awesome.

Note that the spec changes for this were pretty involved. I'm unsure what that translates to in terms of jsdom changes, exactly, but we should probably at least pass any of the web platform tests that got updated.... although it looks like maybe the spec changed before we had a tests-required policy in WHATWG :-/.

• whatwg/dom@625a074
• whatwg/dom@0e17e05
• whatwg/dom@dc3990d

Looks like there’s some weird circular dependency issues.

Are they the same issues as #1247 (comment) ?

Could we merge attributes.js and Attr-impl.js to fix them?

Could we merge attributes.js and Attr-impl.js to fix them?

I’d rather fix WebIDL2JS to generate code that doesn’t completely break on circular require(…)s by creating properties on exports, rather than re‑assigning it: jsdom/webidl2js#154.

I'd rather avoid the circular requires; they're a sign of bad architecture, and I'm not willing to change our tools to make them easier.

review?(@domenic): The tests are now passing.

As stated previously I want us to remove the circular dependency before making any changes in this area.

That will just create another circular dependency, this time between NodeImpl and AttrImpl.

This is because attributes.js is also required by Node‑impl.js:

Since the methods and properties inherited from Node branch on the node type.

It seems very doable, you just have to not keep all the functions together. E.g. move attributeListsEquals and setAnExistingAttributeValue into Node-impl.js.

setAnExistingAttributeValue(…) transiently depends on changeAttribute(…), the latter of which is depended upon by ElementImpl, setAttributeValue(…) and possibly other places, this means that I have to re‑export changeAttribute(…) from attributes.js.

This is looking pretty good. Do you have any thoughts on the compareDocumentPosition changes I linked to?

@domenic Where can I find the compareDocumentPosition(…) changes you mentioned?

The three commits linked in #2770 (comment)

Ping; any thoughts about compareDocumentPosition()? Do you think we should merge this and work on that later?

In fact, using madge to measure dependency cycles, I’ve determined that moving copyAttributeList to living/node.js increases the number of circular dependencies from 9 to 10.

I am confident that the circular dependencies can be detangled. If you cannot do it, that just means we'll have to wait on someone who can.

I’ve managed to get the number of circular dependencies down by 1, when compared to master, by removing the following one, as reported by madge:

1. jsdom/living/generated/Attr.js > jsdom/living/attributes/Attr-impl.js > jsdom/living/attributes.js

I did it by exposing _createAttribute on DocumentImpl, which doesn’t apply strict XML validation on the attribute name, which allowed me to use the existing Document‑based workaround used inside clone.

Regardless of my frustration about the process of working with you on this, I do want to say thank you for taking it on, and that I am happy with the end result. It's a pretty good improvement that solves a ~3 year old bug report.

Thanks as well to @TimothyGu for stepping in when I was burning out on the process.