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

Proposed Breaking Change: PR #796, fix(dom): do not duplicate root element with no id #798

Open
bloodyKnuckles opened this issue Apr 2, 2018 · 4 comments
Open

Proposed Breaking Change: PR #796, fix(dom): do not duplicate root element with no id #798

bloodyKnuckles opened this issue Apr 2, 2018 · 4 comments
Labels
issue is breaking suggestion priority 3 (should) scope: dom

Comments

@bloodyKnuckles
Copy link
Contributor

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

It was discovered when targeting the HEAD element, discussed here, that the DOM driver is duplicating the target container when no ID is provided with it. For example, targeting the HEAD tag:

function main () {
  return {
    head: xs.of(head([
      title('About Page'),
      script({attrs: {src: '/assets/client.js'}})
    ]))
  }
}
run(main, {
  head: makeDOMDriver('head')
}

...currently results in:

<head>
  <head>
    <title>About Page</title>
    <script src="/assets/client.js"></script>
  </head>
</head>

This, of course, is not limited to the HEAD tag but any same-name tag that has an ID provided to neither the target container nor the root VDOM element. For example, given:

<!DOCTYPE html>
<html>
<body>
<main></main>
<script src="bundle.js"></script>
</body>
</html>

...and

function mainFunc () {
  return {
    DOM: xs.of(main([
      h3('About Page'),
      div('page content')
    ]))
  }
}
run(mainFunc, {
  DOM: makeDOMDriver('main')
}

...currently results in:

<body>
<main>
  <main>
    <h3>About Page</h3>
    <div>page content</div>
  </main>
</main>
...

Right now this can be overcome by including an empty string ID with the root VDOM object (whether head or main, or div...):

    DOM: xs.of(main('#', [
      h3('About Page'),
      div('page content')
    ]))

This breaking change DOES NOT affect situations where the target container and root VDOM have different tag names. Given the fix proposed in PR #796, and:

<!DOCTYPE html>
<html>
<body>
<div></div>
<script src="bundle.js"></script>
</body>
</html>

...and

function main () {
  return {
    DOM: xs.of(p('page content'))
  }
}
run(main, {
  DOM: makeDOMDriver('div')
}

...results in:

<body>
<div>
  <p>page content</p>
</div>
...

And this breaking change DOES NOT affect situations where the target container OR root VDOM have an ID set. Given the fix proposed in PR #796, and:

<!DOCTYPE html>
<html>
<body>
<div id="app"></div>
<script src="bundle.js"></script>
</body>
</html>

...and

function main () {
  return {
    DOM: xs.of(div('page content'))
  }
}
run(main, {
  DOM: makeDOMDriver('#app')
}

...results in:

<body>
<div id="app">
  <div>page content</div>
</div>
...

...and

function main () {
  return {
    DOM: xs.of(div('#app', 'page content'))
  }
}
run(main, {
  DOM: makeDOMDriver('#app')
}

...results in:

<body>
<div id="app">page content</div>
...

This breaking change DOES affect situations where the target container and root VDOM have the same tag name and neither have an ID set. For example, given the fix proposed in PR #796, and:

<!DOCTYPE html>
<html>
<body>
<div></div>
<script src="bundle.js"></script>
</body>
</html>

...and

function main () {
  return {
    DOM: xs.of(div('page content'))
  }
}
run(main, {
  DOM: makeDOMDriver('div')
}

...results in:

<body>
<div>page content</div>
...

...compared to WITHOUT the proposed fix results in:

<body>
<div>
  <div>page content</div>
</div>
...
@bloodyKnuckles bloodyKnuckles mentioned this issue Apr 2, 2018
Open
3 tasks
@jvanbruegge
Copy link
Member

jvanbruegge commented Apr 2, 2018 •
edited
Loading

Very good summary, I was reluctant to agree to this change, but now I do

@staltz
Copy link
Member

staltz commented May 1, 2018 •
edited
Loading

Yep, I'm on the same page. Good job! (I'm late to go through all the issues, but I eventually do 😃)

I think we could even include your summary in the release notes. Or if not, then a link to this issue.

@IssueHuntBot
Copy link

@issuehuntfest has funded $40.00 to this issue. See it on IssueHunt

@IssueHuntBot
Copy link

@bloodyKnuckles has submitted a pull request. See it on IssueHunt

@jvanbruegge jvanbruegge mentioned this issue Dec 12, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
issue is breaking suggestion priority 3 (should) scope: dom
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@staltz @jvanbruegge @bloodyKnuckles @IssueHuntBot