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

flatten child arrays to allow nesting #10

Merged
merged 2 commits into from
Jul 21, 2016

Conversation

ds300
Copy link
Contributor

@ds300 ds300 commented Jul 8, 2016

It seems that snabbdom does not allow nested arrays within the children array. This means you can't write code like, e.g:

const todos = [{title: "buy some bread"}, ...];
const renderTodo = ({title}) => <li>{title}</li>;
const todosList = (
  <ul>
    <li>sticky todo</li>
    { todos.map(renderTodo) }
  </ul>
);

because this essentially translates to

h('ul', [h('li', 'sticky todo'), [h('li', 'buy some bread')]]);

Note that the second item of the ul's children array is itself an array. Snabbdom doesn't like this and fails silently, rendering the string 'undefined' in place of the array.

Also note that

const todosList = (
  <ul>
    { todos.map(renderTodo) }
  </ul>
);

works perfectly well because if there is only one child and it is an array, snabbdom-jsx uses it as the children array.

Perhaps this is the intended behaviour. My use case is probably somewhat rare and maybe it's fine to fall back to h to handle such cases. Anyway I figured I'd offer this as a PR because it's not much code and shouldn't touch perf outside of extreme cases.

}

function maybeFlatten(array) {
console.log("mmmaybe")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yikes! Sorry about that.

@jvanbruegge
Copy link

jvanbruegge commented Jul 18, 2016

this is actually very important, because that is a core JSX feature, please merge this as soon as possible

EDIT: manually downloaded the file and can confirm: Works as expected, thumbs up!

@yelouafi yelouafi merged commit 57dfdee into snabbdom-jsx:master Jul 21, 2016
@yelouafi
Copy link
Collaborator

thanks

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 this pull request may close these issues.

None yet

3 participants