Undesirable to override/overwrite Math.random

[magwo]

As far as I can see, this library overrides or overwrites the Math.random() function.
I don't really see the point. This can very easily be done by the user of this library if desired, but has a lot of potential problems for every other user.

I'd much rather see this library be stand-alone and not monkey-patch the Math object.

[windgazer]

As I always check the current issues before I consider using a new lib, this issue seemed easily solved by RTFM, after all, as quoted from the README on the root of the package:

// Use "new" to create a local prng without altering Math.random.
var myrng = new Math.seedrandom('hello.');
console.log(myrng());                // Always 0.9282578795792454

The documented convenience methods of overriding the Math.random() are probably what most lazy developers are looking for though.

[magwo]

Well I disagree and it has nothing to do with RTFM-ing. It's an architectural consideration. Monkey-patching a global, standard library object is not what most programmers want I think. It is perfectly acceptable to do it in a smaller project where all the consequences are known.

At least the library should not do it by default, but instead provide a function to install it into the Math object. For example:

seedrandom.monkeyPatchMathObject()

[windgazer]

I think it's rather brilliant to have written the library in such a way that the signature of the library is consistent all the way through and monkey-patching / stand-alone use is simply regulated by wether or not you use the new keyword.

The 'default' in this case is a matter of documentation. As a matter of usability I think it was a well-chosen way of documenting. For a rookie developer it could well be invaluable to have monkey-patching built in and to have it documented well and clear. For a seasoned developer it won't be in the way and documented well enough to understand.

[magwo]

So a default is always fine and good as long as it is well documented?
Including a library in your application erases your hard drive? It's fine because it says so in the docs.

I still disagree.

Edit: and to clarify.. I think that it's undesirable and problematic even to install the seedrandom() function in the Math object.

[windgazer]

Ignore my previous posts, we're getting nowhere fast.
It is you, as a coder, that instantiates seedrandom in one of two ways:

1. Math.seedrandom('hello.');
2. var myrng = new Math.seedrandom('hello.');

Option 1: And I quote you, 'provide a function to install it into the Math object.'
Option 2: And quoting you again, 'see this library be stand-alone and not monkey-patch the Math object'

There's nothing here to 'disagree' with, it's just a statement of facts.

How do the mentioned code-examples fail to solve your original post?

[magwo]

The function called seedrandom is monkeypatched into the Math object. It adds a function to a global object that is part of the standard library. The Math.random() patching is the second problem.

It's not a big deal, it's just unnecessary and poor style I believe. I'd rather it polluted the global namespace as expected, not the standard library.

[davidbau]

Agreed that it's not the cleanest design, but it's been used this way by a bunch of library users for 5 years, so I'm going to keep it compatible with the way it has always worked since 1.0.

Reopen if patching Math.seedrandom is causing functionality problems interacting with some other tool etc.

[DevBrent]

I just tracked down a major issue with deterministic array shuffling coming out of our Fisher-Yates shuffle which I've traced to this plugin through the use of the https://github.com/wwwtyro/cryptico/ library. Cryptico did not advertise it was overwriting Math.random() with the 2.0 version of your module. We use it for some encryption in transit on top of our SSL transmission chain.

Extremely disappointed by how easily the module encourages monkey patching the standard Math library especially in cases where you can unexpectedly and predictably seed Math.random().

Surprisingly brief code sample that shows how your decisions can impact third-party apps without being properly documented:
wwwtyro/cryptico#20 (comment)

const cryptico = require('cryptico');
const encryption_key = cryptico.generateRSAKey('uZq%ogoYtt^%', 1024);
const predicted_value = 0.006809886770680776;
const rand = Math.random();
if (rand === predicted_value) {
  console.log('I can\'t believe you\'ve done this.', rand);
}

[windgazer]

You know, it's still an optional feature of this library, fix cryptico to use this lib by using new instead, it's probably less than 5 minutes of work...
On the flip-side, if you're using a crypto built by a developer who did not take into account the ramifications of how he's using a seeded random function (which can actually be used without overwriting the default random behavior), you might want to question it's functionality?

[magwo]

No.

Seriously, it is widely regarded as extremely bad practice to permanently alter the global standard objects, regardless of whether it is done by a library or application code.

The overwriting of the random function in the global Math library is not a feature - it is a problem. If this, occasionally, is a desired side-effect for someone using this library, it can be easily done using a one-liner.

[davidbau]

cryptico is a good example of the problem. Though it is in such a weird context that one wonders whether it is an accident or a malicious intentional bug.

But it would be good to avoid causing this problem by accident. Maybe v3.0 should break the API and require tests to say Math.random = Math.seedrandom('testing'), though people might also complain if a new version contains nothing new but a gratuitious API change that breaks their old unit tests.

Chime in again if you find any other examples of bad usage in bad places. For now I'm assuming it is a rare problem. I have updated the README to include an explicit warning.

[SReject]

A few things I'd like to add to this discussion

First, its considered bad practice to ambiguously alter native instances. Its not about resolving current conflicts but rather future proofing against conflicts. The general rule is to leave native objects pristine and, dependent on environment, add the library to the global object or return it as a module export.

Second, the reason for versioning is so that a user-base can choose which version to (continue) using. It is understood by the community that a major version increase is not indicative of new feature sets but rather changes that alter previously defined behaviors.

NPM uses major-version bumps to indicate backwards compatibility changes and your cnd requires a version in the URI. As such, I doubt anyone would be disappointed with a v3.x release that contained only a change to the API.

[Herover]

If it's always considered bad practice to use this library to override Math.random outside testing and development, it could print something like Warning: seedrandom overrides Math.random, please read https://github.com/davidbau/seedrandom/issues/20. Nothing breaks and users of this library and libraries that misuse it in production is likely to be notified about the problem.

[davidbau]

We can avoid printed warnings, I think, by just fixing the API. How about this, since it's 2019:

1. Remove definition of Math.seedrandom as a side-effect of var sr = require('seedrandom') in nodejs.
2. Usage remains unchanged for plain-script users. In that context, Math.seedrandom is still defined.
3. Version bump to 3.0.0 as a way of warning about an incompatible API change.

Reasonable? The master branch reflects this. My thought is to push a version bump sometime in March.

[magwo]

1. Remove definition of Math.seedrandom as a side-effect of `var sr = require('seedrandom')` in nodejs.

2. Usage remains unchanged for plain-script users.  In that context, Math.seedrandom is still defined.

3. Version bump to 3.0.0 as a way of warning about an incompatible API change.

Edit: Actually, I would really encourage you to, in version 3.0, remove the helper function that overwrites Math.random and also the one that adds Math.seedrandom. Put the functions in a global Seedrandom object instead, for plainscript users. Monkey-patching the standard Math lib is out of scope of this library in my opinion, and encourages a risky operation. The users most interested in such functionality are probably the ones least aware of the problems it may cause.

To be clear, this is what I'm referring to: https://github.com/davidbau/seedrandom/blob/released/seedrandom.js#L95

Removing the monkey-patching will obviously be API breaking for some plainscript users, and I think it would be healthy for those users to explicitly add this to their code when they upgrade to 3.0:

var myRng = new Seedrandom("hello");
Math.random = myRng;

Otherwise looks like a good plan to me!

[davidbau]

I do not want to force people to touch old code for no good reason (from their point of view).

On the other hand, I'm 100% in favor of encouraging people to write better future code.

Newer usage with require('seedrandom') should not include the monkey-patching form, and the proposed change fixes this. Note that line 95 is not reachable line 245 is not executed. https://github.com/davidbau/seedrandom/blob/master/seedrandom.js#L245

[davidbau]

To encourage future code to use the local prng form, I've updated the readme in master.

https://github.com/davidbau/seedrandom/blob/master/README.md

[magwo]

I do not want to force people to touch old code for no good reason (from their point of view).

They would only have to if they upgrade to 3.0 version, however, which means they are already touching the code, as I see it. Whichever path you choose, this is a good change overall.

[davidbau]

Relase 3.0.0 has an updated readme and removes the globals in require-usage.