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

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

Open
RichardBradley opened this issue Dec 17, 2015 · 0 comments · May be fixed by #936
Open

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

RichardBradley opened this issue Dec 17, 2015 · 0 comments · May be fixed by #936
Labels

Comments

@RichardBradley
Copy link
Contributor

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.

RichardBradley added a commit to RichardBradley/ng-admin that referenced this issue Feb 17, 2016
RichardBradley added a commit to RichardBradley/ng-admin that referenced this issue Feb 17, 2016
@RichardBradley RichardBradley linked a pull request Feb 17, 2016 that will close this issue
RichardBradley added a commit to RichardBradley/ng-admin that referenced this issue Feb 17, 2016
RichardBradley added a commit to RichardBradley/ng-admin that referenced this issue Feb 17, 2016
@Kmaschta Kmaschta added the bug label Nov 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants