djvirgen / virgen-acl

A simple ACL for node.js
MIT License
74 stars 19 forks source link

browser compatible array check #5

Closed smhg closed 11 years ago

smhg commented 11 years ago

Array.isArray is unavailable in some browsers, this should make things work everywhere.

Secondly there seems to be a trimming difference (my editor trims end of lines automatically). Just let me know if you don't want this changed.

Please note: npm test seems to rely on coffeescript and an old version of node (they won't run on 0.10.13).

djvirgen commented 11 years ago

This is great! Thanks for the pull request. I really appreciate your effort in making this library compatible with browsers :)

Just one suggestion -- can you update it to use the ECMAScript standard for checking if a variable is an array?

if (Object.prototype.toString.call(someVar) === '[object Array]') {
  alert( 'Array!' );
}

Please refer to this SO question for details.

Also I'll take a look at the node version / coffeescript issues. Thanks again!

djvirgen commented 11 years ago

I've removed the coffeescript requirement from mocha, and relaxed the mocha version requirement. Let me know if that helps npm test with node 0.10.13. It seems to work with version 0.10.12, which is the latest version available in homebrew.

These changes have been published to NPM in version 0.0.18. Thanks again for reporting the issues!

djvirgen commented 11 years ago

BTW I'm getting three spec failures on your branch:

1) acl action should allow all actions specified in array:
   AssertionError: expected false to equal true

2) acl action should allow all actions specified in array:
   AssertionError: expected false to equal true

3) acl action should allow all actions specified in array:
   AssertionError: expected false to equal true

Let me know if you have any trouble getting the specs to pass. Thanks!

smhg commented 11 years ago

Thanks for your remarks!

I had to update mocha (and did the same for should) to make the tests work on windows (old version caused a process.nextTick warning). So it was not directly related to the node version. As you very rightfully pointed out, I now used the proper isArray alternative (copied from MDN).

The tests succeed now. Maybe you can add the project to Travis and add the badge to the Readme?

Small note: I did a rebase to remove my old commit and keep your log clean. I hope this doesn't cause issues (I'm not very experienced with git).

djvirgen commented 11 years ago

The latest change look great, thanks! :)

djvirgen commented 11 years ago

Your pull request has been merged in and version 0.0.19 has been published to NPM. Thanks again for contributing! :)