eli-darkly
commented
2 years ago On the one hand, monkey-patching Array in particular is an uncommon practice that, as you say, there are some pretty strong opinions about. In fact I think it's safe to say that most people would consider it a bad practice; it has consequences that monkey-patching other classes would not, because JS developers are accustomed to the idea that other kinds of objects can have spurious properties that come from their prototypes (and would therefore always use either for...of or Object.hasOwnProperty to iterate named properties), but JS developers generally will assume that an ordinary array is just an array. So I would certainly be strongly against any library developer doing such a thing.
When an application developer does it, they can at least take responsibility for making sure their own code is safe. Your request here is for the library code to also be adjusted to tolerate this practice. I think, on the general principle of programming defensively against an unknown environment, that's probably reasonable for us to do— as long as we can enforce it via the linter to make sure we don't accidentally re-introduce unsafe usages, which I'm fairly sure is doable. However, we definitely cannot guarantee that the SDK will be resilient against monkey-patching of built-in JS types in general. It's too hard to anticipate every potential consequence. Probably the best we can do is to adjust the for usage.
Of course, even if the LaunchDarkly SDK is made to tolerate this, that doesn't mean it won't cause problems in any of the many other NPM packages you might be using, so I would still strongly recommend migrating away from this approach in the future.
luke-schleicher
Context: I am running a nodejs server that I was trying to integrate this library into.
However, our server uses some monkeypatched extensions of Arrays for convenience. Functions like
first,last,pluck, etc (Array.prototype.first = function() {...})When trying to integrate this library into my project, it failed on the call to
init, because this code was iterating not only over the indices in the arrays (0, 1, 2, etc), but also the names of our extension (first, last, pluck, etc).This obviously threw an error when it tried to call
clause.opon a Function.I'm sure there's opinions out there either way on the practice of monkeypatching a global object type like Array, but regardless this made it much harder to integrate launchdarkly because I had to go through and remove all our extensions from the code.
As far as I know,
for ... of ...is the recommended method to iterate over arrays as well.I've gone ahead and changed all the instances of
for ... in ...that were preventing library initialization. There may be morefor ... in ...that should be converted tofor ... of ..., but I suppose half the purpose of this PR is to draw your attention to this issue and get a conversation going.Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Provide a clear and concise description of what you expect to happen.
Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.