Skip to content

Iterating over array entries using "for .. in" breaks if Array polyfills are used  #863

@RichardBradley

Description

@RichardBradley

See marmelab/admin-config#57

The ng-admin project uses lots of unguarded "for..in" statements, e.g. https://github.com/marmelab/ng-admin/blob/master/src/javascripts/ng-admin/Crud/list/ListController.js#L59

All of these need to be converted to use angular.forEach or add a if (!xs.hasOwnProperty(i)) continue; guard.

We also need to merge in the fix from marmelab/admin-config#58

I couldn't get any unit tests to fail for this bug, but the following diff should allow you to repro the issue:

diff --git a/src/javascripts/test/before_all.js b/src/javascripts/test/before_all.js
new file mode 100644
index 0000000..96f1ee4
--- /dev/null
+++ b/src/javascripts/test/before_all.js
@@ -0,0 +1,16 @@
+
+if (!Object.prototype.test_prototype_entry) {
+    Object.prototype.test_prototype_entry =
+        "Don't use for..in to enumerate Object properties, as users are free to " +
+        "add entries to the Object prototype, for example for polyfills. " +
+        "You should instead use\n" +
+        "  for (let i in xs) { if (!xs.hasOwnProperty(i)) continue; var x = xs[i]; ... }";
+}
+
+if (!Array.prototype.test_prototype_entry) {
+    Array.prototype.test_prototype_entry =
+        "Don't use for..in to enumerate Array properties, as users are free to " +
+        "add entries to the Array prototype, for example for polyfills. " +
+        "You should instead use\n" +
+        "  for (let i in xs) { if (!xs.hasOwnProperty(i)) continue; var x = xs[i]; ... }";
+}
diff --git a/src/javascripts/test/karma.conf.js b/src/javascripts/test/karma.conf.js
index 0c28df7..142f8f3 100644
--- a/src/javascripts/test/karma.conf.js
+++ b/src/javascripts/test/karma.conf.js
@@ -22,6 +22,7 @@ module.exports = function (config) {

             'ng-admin.js',
             'test/function.bind.shim.js',
+            'test/before_all.js',
             'test/unit/**/*.js'
         ],
         plugins: ['karma-webpack', 'karma-jasmine', 'karma-chrome-launcher', 'karma-phantomjs-launcher'],

Then any tests which assert on the result of the "for..in" linked above should fail. It seems that no unit tests currently check the output closely enough to notice this bug.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions