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

Setting NodeList length in strict mode throws TypeError #44

Closed
akre54 opened this issue Dec 4, 2015 · 7 comments
Closed

Setting NodeList length in strict mode throws TypeError #44

akre54 opened this issue Dec 4, 2015 · 7 comments

Comments

@akre54
Copy link

akre54 commented Dec 4, 2015

I'm updating an app from d3 3.5.x and ran into an issue using selections in strict mode.

My code is a basic selection / data binding: selection.selectAll('div').data(data). Data is an Array and the selection length is equal to the data length (4).

This worked correctly in 3.5.x with strict mode. Now I'm getting a TypeError in the data method when it tries to set update.length = dataLength (src).

Here's a reproducible test case, but the basic gist is this:

var nl = document.querySelectorAll('div');

console.log(nl.length); // 4
nl.length = 4;
console.log(nl.length); // 4, doesn't change length but silently swallows error

'use strict'
nl.length = 4
// TypeError: Cannot set property length of #<NodeList> which has only a getter
@mbostock
Copy link
Member

mbostock commented Dec 4, 2015

I’ll investigate, but a word of caution: as the README says, this module is currently an experimental API and is likely to change before 4.0 is released. This module and d3-transition are likely to be the last of the 4.0 modules to be released (since, in some sense, they are the most important to get the API right). So I wouldn’t recommend using this module in production yet.

I am working on implementing the other modules first, so I can’t be as responsive fixing issues in the unreleased modules such as this one.

That said, it is a bug if the code is trying to set the length of a NodeList, clearly. It should have arrayify’d the arrays first before mutating them.

@mbostock
Copy link
Member

mbostock commented Dec 4, 2015

Also, it looks like you’re trying to call selection.text on the enter selection before appending. That shouldn’t fail, but it also won’t have any effect because you’re setting the textContent property of a placeholder node. You’ll want to say something like this instead:

selectAll('p')
    .data(data)
  .enter().append('p')
    .text(d => d)

@akre54
Copy link
Author

akre54 commented Dec 4, 2015

Sure thing, this isn't a production app, so no damage there. The new stuff is too tempting to leave alone though :)

Also, the example was thrown together, that's not app code, the failure is coming in data, before it even hits the enter section but I'll fix the example.

As far as I can tell, the enter and exit calls are arryifying the selection around emptyNode, but the update call is simply this._root, which is a NodeList.

I'm really looking forward to using this stuff in production, happy to help bugbash if that's in the cards.

@mbostock
Copy link
Member

mbostock commented Dec 4, 2015

Cool, just making sure. Another gotcha in your test case (probably unrelated to the bug) is that you didn’t select a node to append to, so your entering paragraph elements will get added to the document element rather than the body. The fix:

select('body').selectAll('p')
    .data(data)
  .enter().append('p')
    .text(d => d);

@akre54
Copy link
Author

akre54 commented Dec 4, 2015

Ah. Yeah in the app its a normal selection on a group of bar charts (similar to the crossfilter example). Works right if I leave d3.selectAll in there.

@mbostock
Copy link
Member

mbostock commented Dec 4, 2015

The bug is only exhibited when calling selection.data on a top-level (flat) selection, rather than one nested under a parent element. That typically never happens because you instead specify a parent to append to. But it shouldn’t fail. Fix incoming.

@akre54
Copy link
Author

akre54 commented Dec 4, 2015 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants