fix(dom): do not duplicate root element with no id

[bloodyKnuckles]

Do not duplicate the root element when not using ID property. For example, without this PR, when simply targeting the HEAD tag it gets duplicated because there is no associated ID. This PR simply gives the selId variable a default value of empty string, which correlate it with the default value of empty string given to the ID property of the rootElement object by document.querySelector.

• There exists an issue discussing the need for this PR
• I added new tests for the issue I fixed or built
• I used make commit instead of git commit

Discussions here: #792 #540

This fix broke two other tests:

DOMSource.events() should catch bubbling events in a DocumentFragment
DOMSource.events() should catch non-bubbling events in a DocumentFragment with useCapture

These tests worked before because the root tag was duplicated since there was no ID assigned to either the root element nor the parent VDOM object. After fixing the "duplicate bug" the root tag was no longer duplicated causing the tests to fail.

...

return {
  DOM: xs.of(div([div('.clickable', 'Hello')])), // <-- no ID
};

...

const renderTarget = fragment.appendChild(document.createElement('div')); // <-- no ID

I added an ID to the root VDOM objects—now all tests pass.

...

return {
  DOM: xs.of(div('#bubpar',[div('.clickable', 'Hello')])), // <-- add ID 'bubpar'
};

Also, to get a working test I added a <footer> tag to the test index.html file, in order to be able to get a "clean" root element and parent VDOM object. My attempts to use createRenderTarget failed to test the fix, my guess is because dynamically adding a DOM object for the root element always created an (empty string) ID, rather than the needed undefined—and using the HEAD tag broke most tests.

I realize the broke "bubbling" tests showed me how to test the fix without adding the <footer> tag to the test index.html file. So I cleaned the test up.

Glad to be learning these tools so thanks for the guidance and let me know if I need to do anything different.

[staltz]

Good to see the thought you put into this. Nice, @bloodyKnuckles :)

Before merging, we still need a few things:

• Commit messages follow our format
• Rebase the commits to remove multiple commits
• Discuss the breaking change

Because one test was modified, this means a breaking change. There are 2 things to consider, I think: (1) could we possibly support this new feature without breaking anything? Like, could we only support this feature in the HEAD tag? (2) If not, can we consider what are all the possible breaking changes this will cause and how to best communicate it in a migration guide? E.g. does this mean that nested vnodes like div([div([div([..... will be squashed into one div([?

[bloodyKnuckles]

I appreciate the opportunity to get up to speed on this type of collaboration—especially on this project.

I need more help. I think I merged the previous commits but don't know how to check that to be sure.

Also, I added a new test that verifies nested, same-name elements are not squashed into one. I.e.,

div([div([div('dummy text')])])

...actually produces:

<div>
  <div>
    <div>dummy text</div>
  </div>
<div>

And I started an issue for discussion, issue #798: Proposed Breaking Change: PR #796.

[staltz]

Good job @bloodyKnuckles and sorry for the delay in reviewing it.

I'm curious about travis builds failing though. It usually "fails" for random reasons with Sauce Labs, but this time one of them looks like a real failure:

not ok 442 Chrome 49.0 - DOMSource.events() should catch non-bubbling events in a DocumentFragment with useCapture since it's one of the tests that were updated. I'll restart the travis build. We could also see in BrowserStack what's going on with Chrome 49

[jvanbruegge]

@bloodyKnuckles are you still interested in persuing this? This PR would need a rebase and the failure would have to be debugged
Otherwise I would tackle this

[jvanbruegge]

(I investigated that issue myself and the change still causes timeouts and other issues in the document fragment tests)

[bloodyKnuckles]

Please, remind me, how do I run the tests on my branch?

[jvanbruegge]

After the rebase, just by using pnpm test. The tests fail in all browsers, so you dont need the browserstack infrastructure for that. If you get your tests to pass and commit, the CI will run normally

[bloodyKnuckles]

I'm trying to locally restore my pull request branch. Before I run the tests I am installing but executing sudo pnpm install gives me a bunch of:

WARN  Fetching registry.npmjs.org/... failed

...errors. This takes a long time then ends with a bunch of:

Error: EISDIR: illegal operation on a directory, open...

...errors.

I'm using sudo and the directory owner is root.

Executing pnpm config get registry returns:

https://registry.npmjs.org/

I tried changing that to http://registry.npmjs.org/ but did not help.

[jvanbruegge]

You need to delete previous node_module installations. And before running the tests you have to rebase, we changed the whole testing setup in thr meantime