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

IE8 fix: filter __proto__ from for..in loops #2209

Closed
wants to merge 2 commits into from
Closed

IE8 fix: filter __proto__ from for..in loops #2209

wants to merge 2 commits into from

Conversation

shawnbot
Copy link

D3 is currently out of commission in IE8 thanks to some tight for..in loops in the d3.map() implementation. When d3.js is executed, IE8 fails while iterating over d3_svg_lineInterpolators when it hits the __proto__ key:

image

Because, unfortunately, in IE8:

for (var key in Object.create({})) console.log(key);
// prints: "LOG: __proto__"

Which means that we have to use hasOwnProperty() instead of just the in operator. This PR suggests one fix (for which the tests pass, and I've confirmed it works in IE8), but it might have performance implications. For instance, instead of the locally bound has(key) these functions could call this.has(key) instead, which might be faster.

I haven't looked too closely to see if there are any other places in d3 where this might apply, but the d3.map constructor might be a candidate.

@shawnbot
Copy link
Author

Correction: calling this.has(key) would only work if the has() method used hasOwnProperty().

@shawnbot
Copy link
Author

I noticed that a lot more discussion about this has taken place in #2099, and that that issue was closed. I know that IE8 isn't exactly at the top of your list, but this is a pretty simple fix. I'd be happy to even refactor this a bit so that IE8 (using feature detection, of course) gets a different/slower version of the functions, so that this compatibility fix doesn't affect performance in modern browsers.

@sylvinus
Copy link

sylvinus commented Apr 1, 2015

+1 for this issue. Would also love a simple fix to avoid breaking at load time, I confirm this works on IE8:

@@ -8146,6 +8146,7 @@
     monotone: d3_svg_lineMonotone
   });
   d3_svg_lineInterpolators.forEach(function(key, value) {
+    if (!value) return;
     value.key = key;
     value.closed = /-closed$/.test(key);
   });

@shawnbot
Copy link
Author

shawnbot commented Apr 1, 2015

FWIW, @sylvinus, aight now ships with an aight CLI tool that you can use to prevent the errant for..in iterations over __proto__ in any JS file. The included d3.ie8.js is v3.5.3, but you should be able to create an up-to-date build with:

npm install -g aight
curl -s http://d3js.org/d3.v3.js | aight >  d3.v3.ie8.js

@mbostock mbostock added this to the Icebox milestone Oct 22, 2015
@mbostock
Copy link
Member

Not that it matters, but does IE8 log __proto__ with Object.create(null) rather than Object.create({})?

For 4.0, I am currently planning on ditching the Object.create(null) for d3.map, and instead go back to the simpler prefixed properties; see src/map.js in d3-arrays. In addition to the __proto__ issue (which also existed on Node 0.10), non-prefixed keys that happened to look like numbers sometimes caused maps to behave like arrays, and then triggered large memory allocations. So forcing the keys to always look like strings rather than numbers seems like a safe choice. And the code is simpler.

So with apologies to Shawn (who I have neglected for months by not replying to this pull request), but I’m trying to minimize the amount of changes to 3.x and focus most of my effort getting 4.0 released.

@mbostock mbostock closed this Oct 22, 2015
@shawnbot
Copy link
Author

No worries, Mike! IE8 support is basically off my radar now that global and government site usage is <2%. I'm also going to use custom 4.0 bundles from now on to save precious kb's, so 👍

@mbostock
Copy link
Member

👍

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

Successfully merging this pull request may close these issues.

3 participants