Modifying built-in Array prototype potentially causes collisions

[sabtt]

I ran into an issue while using this library whereby another library had also added a .random() function to the built-in Array prototype. The other implementation did not select a random item but randomized the Array (in-place) breaking various functionality in rot.js.

I have replaced all instances of Array.random() and Array.randomize() with the local functions getRandomItem(Array) and getRandomizedArray(Array), which seems to have fixed the issue.

Generally modifying built-in prototypes is considered bad practice for this reason and a couple of others - there are several other instances of this in the library, but as these were not causing issue for me I have not removed them.

[ondras]

Hi @sabtt,

thanks for your efforts and your PR. My opinion regarding this issue is basically the same as it was in #46 (worth reading), so unless it is absolutely necessary, I am not really willing to step down to the other library, whose naming choice I consider sub-optimal.

If you let me know what library has a conficting Array.prototype.random implementation, I will try to get in touch with them and try to convince them that randomize might be a better name.

This is for the very first time this has ever happened in rot.js (as #46 turns out to be caused by something different), so while I understand the potential of implementation collisions with built-in prototypes, the status quo does not give enough evidence for me to change my mind re. Array.prototype.random.

[sabtt]

That seems a strange position to take - what is the benefit of modifying the prototype? The negative in this instance is obvious, and the fix straightforward.

I'm not suggesting the other library (https://github.com/petersirka/nosql/blob/9eaa2b5690db6cfb80994c8aa259e4b2fd40b15b/utils.js#L492) is correct and you should be subservient to its implementation - as in my initial comment I was saying it is generally bad practice for anyone to modify. In fact, I was also putting together a pull request for that library to the same end. There is no benefit, as far as I can tell, to modifying a global prototype when a local function can do the same without the obvious drawbacks.

This is especially true of rot.js - since it uses its own PRNG it could not use another library's implementation. You're essentially mandating that not only should Array.random/randomize should use your implementation, but also your PRNG. In this instance, checking for existence of the prototype method before assigning it is not enough - it must be overwritten with your version to ensure rot.js works as expected.

[ondras]

That seems a strange position to take - what is the benefit of modifying the prototype? The negative in this instance is obvious, and the fix straightforward.

Because it is consistent with how the language works. JS is a multi-purpose multi-paradigm language, but when the ES committee decided to add string padding, they accepted String.prototype.padStart and not padStart(string). Having type-specific methods declared in their prototype chain feels natural to me.

My view is this: in an ideal world, String.prototype.random gets either standardized or all competing implementations use identical semantics (pick a random item from an array). And my own implementation aims towards this goal.

There is no benefit, as far as I can tell, to modifying a global prototype when a local function can do the same without the obvious drawbacks.

Apart from the consistency viewpoint, having stuff globally (namespaced) available means that you do not have to care about importing the behavior.

This is especially true of rot.js - since it uses its own PRNG it could not use another library's implementation.

This is an interesting point. I would argue that it is perfectly valid to use a differing implementation (a Math.random-based one, for example) for Array.prototype.random, as it maintains the semantics. The fact that my own random implementation uses ROT.RNG is beneficial mostly when testing stuff, because ROT.RNG can be seeded. But this can be achieved with Math.random as well, by overriding the Math.random property with a custom function.

[sabtt]

I understand where you are coming from, but generally disagree. When there is a universal extension to the language introduced by the ES committee, it makes sense to add to the prototype classes - this is consistent with the existing public API. Again, it would be appropriate when shimming behaviour that has been ratified by the committee but not implemented in all JavaScript runtimes. When implementing a supporting function for your own library, this is absolutely not true. Part of your public API currently is Array.random / Array.randomize, yet this is not documented anywhere, and seems a little off-target for a roguelike toolkit.

You seem to be of the opinion that putting your own functions into the global namespace is positive, desirable behaviour. I would instead characterise this as a flaw of JavaScript - that it is too easy to "pollute" the global namespace with your own functions and objects. Fortunately, it is almost never required. Through the use of closures we can provide "global-like" functions available throughout our own modules without risking conflict with other code. This becomes even more important when using JavaScript where many small, focused modules are the norm - my current project includes in the region of 250 JavaScript dependencies, for example. This would be completely untenable if most had not been designed as ring-fenced modules.

I agree it would be nice if everybody could come to consensus on what functionality should be available to the built-in prototypes provided by JavaScript runtimes, but this seems like an unrealistic goal - look at this conversation, for example, where we both fully understand the background and effects of built-in modification but have reached opposing conclusions! The world we live in is most definitely not ideal so we must compromise.

I am not a purist, and would have no objection to, e.g. you overriding or appending functionality to built-ins within your own projects - I do this myself occasionally. rot.js, however, is intended to be a library incorporated into other people's code, and so changing the JavaScript environment in any fashion is invasive and should not be done unless that is the express purpose of your library.

If you aren't prepared to accept this change (or one like it) on philosophical grounds, I am left having to manually patch your library every time I update or simply stop using it altogether, which would be a real shame because I think it's a great library!

[ondras]

(https://github.com/petersirka/nosql/blob/9eaa2b5690db6cfb80994c8aa259e4b2fd40b15b/utils.js#L492)

Wow, this implementation is, at least strange. I will try to get in touch with them.

Part of your public API currently is Array.random / Array.randomize, yet this is not documented anywhere, and seems a little off-target for a roguelike toolkit.

http://ondras.github.io/rot.js/manual/#js

It occured to me that Roguelike games are full of randomized/procgenerated stuff and having a convenience [a, b, c].random() method is very useful.

If you aren't prepared to accept this change (or one like it) on philosophical grounds

I am willing to make the change if there are no other ways around this issue. I do not see the process of built-in modifications as a black/white scenario; some prototype enhancements are worth it and some are not. My experience, so far, was pretty positive with this approach: the Array.prototype.random was always either unique or consistent with competing implementations. I still think that I chose the name properly and that any reasonable developer shall accept its semantics unambiguously. Let's see what the nosql project thinks about this.

Now the main problem with the change you suggest is that the A.p.{random,randomize} API is already used in the wild by projects that are built with rot.js, so removing it is a breaking, backwards-incompatible change.

Fortunately, rot.js is basically a completed project, so patches/releases are pretty rare these days. This gives me time to somehow solve this issue before you leave rot.js for good :-)

[sabtt]

Oh, apologies, I did look through the docs but missed that section. That does change my opinion of this somewhat. I still think it's a poor choice to use such a generic name for a specific implementation, and would always avoid attaching to the built-in prototypes in a library, unless that was the express purpose of said library. I'd probably have gone with some kind of helper class instead to expose that functionality instead (ROT.Array.random / randomize). What's done is done though, and as you say your dependants may rely on that functionality now. Of course it's a lot easier to patch that functionality back in than remove it, but I wouldn't suggest everyone else made that change just for my benefit!

I've not had chance to look at the nosql code in much detail yet, but my immediate code diving suggests that function isn't even used, it appears to just be a utility library included for other functions. The nosql library is part of a larger project though, so it might be difficult to get it removed / replaced. I don't imagine many people are using rot.js server-side and the nosql library wouldn't be much use in a browser, so it's unlikely others would be hit by this specific conflict.

I'll almost certainly continue to use rot.js regardless of your decision, in fact I may well drop nosql as it was just a convenient solution to persistence, I was just outlining my options so you could see where I was coming from.

[ondras]

@sabtt, NoSQL seems to have fixed this: https://github.com/petersirka/nosql/commit/ce4e39022fb244b9327cb8d114402712f882605c

[sabtt]

Huh, well that was unexpected. Thanks for your discussion and help!