rot.js breaks hashtables (in node.js)

[psvensson]

This took some time to find. But what happens is that rot.js adds properties to arrays, for example the 'random' function.

When you use arrays as hashtables, you will then always get at least one 'hidden' property that will jump pout and be the 'random' function.

This breaks pretty much all of my code (in a message router and ORM built separately, and which works very well with everything else I've used it with) and is really not an expected behavior of arrays.

I would respectfully recommend to rewrite those part of rot.js that extends the prototype of intrinsic JS objects so that they avoid to do so.

Thanks for a very nice project, otherwise.

[ondras]

When you use arrays as hashtables, you will then always get at least one 'hidden' property that will jump pout and be the 'random' function.

You are correct! That is because you shall not use JS arrays as hashtables and you shall not iterate JS arrays via for-in.

The random and randomize enhancements of Array.prototype are non-standard, but implementing other standard methods via polyfills is a recommended practice. This means that any code that does for p in array will run into issues, sooner or later, independently on what rot.js uses.

I can offer you three alternative approaches:

1. Adjust your code so it uses Object.create(null) as a proper key-value storage container;
2. Adjust your code so it iterates using hasOwnProperty (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#Iterating_over_own_properties_only);
3. Send a patch to rot.js that makes these prototypal enhancements non-enumerable.

[hogart]

There's also native Map and WeakMap in almost every environment at the moment.

[psvensson]

Actually I should really switch to Map since years back. Thanks for reminding me!

I still don't agree with making changes to a library which can affect anything not related to it, but this is a very old issue and we know which sides we are on :)

On Sat, Jan 2, 2016 at 10:20 PM, Konstantin Kitmanov < notifications@github.com> wrote:

There's also native Map and WeakMap in almost every environment at the moment.

— Reply to this email directly or view it on GitHub https://github.com/ondras/rot.js/issues/83#issuecomment-168428825.

[ondras]

I still don't agree with making changes to a library which can affect anything not related to it

This is a very reasonable opinion, but JS just does not work this way. Polyfilled Array.prototype.forEach can be found in many libraries with respect to IE8- compatibility -- and people are totally okay with that, even if it breaks the library encapsulation.

Polyfills and monkey-patching are what we get with a language as dynamic as JS. I understand that polyfills are not that common in nodejs world, but keep in mind that rot.js was initially created and designed for client-side usage...

[psvensson]

No, I'm OK with polyfills that giveadditional capabilities, of course. I hope you understand that I'm specifically referring to polyfills that break the JS spec. But there are of course two camps here, as mentioned.

I thought that myself actually (coming from the browser-side), and I appreciate the additional difficulties porting rot-js to node, especially the build system and tests. Good work, even if I think just avoiding the array extensions might have been a reasonable succession. Still neat. I will try to make a more complex tyrant(ish) generator and see if I can make it general enough to return with another PR in spring.

On Sun, Jan 3, 2016 at 8:51 PM, Ondřej Žára notifications@github.com wrote:

I still don't agree with making changes to a library which can affect anything not related to it

This is a very reasonable opinion, but JS just does not work this way. Polyfilled Array.prototype.forEach can be found in many libraries with respect to IE8- compatibility -- and people are totally okay with that, even if it breaks the library encapsulation.

Polyfills and monkey-patching are what we get with a language as dynamic as JS. I understand that polyfills are not that common in nodejs world, but keep in mind that rot.js was initially created and designed for client-side usage...

— Reply to this email directly or view it on GitHub https://github.com/ondras/rot.js/issues/83#issuecomment-168534866.

[ondras]

I hope you understand that I'm specifically referring to polyfills that break the JS spec.

Right. As soon as the random or randomize methods get standardized, I will immediately remove my own variants from Array.prototype (as long as they differ from the official version) :smile: