mycoboco / wcwidth.js

a javascript porting of C's wcwidth()
http://code.woong.org/wcwidth.js
Other
12 stars 2 forks source link

Monkeypatching String.prototype #4

Closed timoxley closed 10 years ago

timoxley commented 10 years ago

Think you could at least expose an interface that doesn't monkeypatch String.prototype?

This may not seem like a big problem, but it breaks node's module system: if there's two versions of this module installed anywhere in the hierarchy, there's no guarantee which one will actually be used… it use whichever version of the module was required most recently, the timing of which may not be deterministic.

mycoboco commented 10 years ago

I've never thought the possibility that adding a getter into String.prototype might break node.js's module system.

One way to fix the problem I can think of is to let a caller decide whether or not to monkey-patch by providing an option; fortunately, it already have an option argument to pass when requireing the module. If you have a better way to avoid monkey-patching a global object, please let me know.

I'll fix it by today.

Thanks for your comment.

mycoboco commented 10 years ago

wcwidth.js now has an option monkeypatch to control its monkey-patching of String.prototype. Closed by 4ba17a2688ba0409434148b51eee0eeb3d9f2e00

timoxley commented 10 years ago

:+1: thanks!

timoxley commented 10 years ago

@mycoboco IMHO it really shouldn't monkeypatch by default. People should have to opt-in to potentially perilous operations… rather than opt-out.

mycoboco commented 10 years ago

@timoxley I fully agree with you. The problem is that, even if there are few packages that depend on wcwidth.js in npm, several closed projects already use it with the overriding getter and I have almost no control over them. As you pointed out, it is very likely to opt-in that problematic construct even when not really necessary, so I'm going to mark using it with the default option as deprecated and to gradually change it to a better structure.

Thanks for your comment.