marmelab / marmelab/ng-admin

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

Open
#863 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

bug
Dominant language
JavaScript
Stars
3.9k
Forks
703
PR merge metrics
No merged PRs in 30d

Description

See https://github.com/marmelab/admin-config/issues/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 https://github.com/marmelab/admin-config/pull/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.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with the unguarded loop in src/javascripts/ng-admin/Crud/list/ListController.js and locate the other for..in statements in the ng-admin sources. Add the prototype-entry setup from the issue to src/javascripts/test/before_all.js and include it in src/javascripts/test/karma.conf.js, then run the affected unit tests. Done means array iteration is unaffected by added Array and Object prototype entries and the referenced admin-config fix is merged.

Written by the indexing model from the issue text.

Assessment

Tech stack
angularjs, javascript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
38/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.