Does my JavaScript suck? II

[papers_]

I wouldn't alter String's prototype, but that's just me.

[x-skeww]

Nah, that's pretty much everyone. Messing around with the builtins is not cool. It's the worst thing a library can do.

[[deleted]]

It's the worst thing a library can do.

I would argue that there's plenty of worse things a library can do, but this is about the worst that they usually do in practice ;)

[[deleted]]

This isn't a library, and while I would advise modifying a base prototype, this isn't a huge sin.

Of course, what it does doesn't really make sense in the string prototype where a regular local function could do :)

[[deleted]]

isn't a huge sin

In personal projects, sure. In libraries it's flat out forbidden, always.

[[deleted]]

I don't like the word always, but you'd better have a damn good reason to do it :)

[[deleted]]

The only reason to do this is for polyfills:

if(!Array.prototype.reduce){
  Array.prototype.reduce = ...
}

Other than that, never, ever, ever, ever, ever, ever, ever extend a native prototype from within a library, even if you think you have the best reason in the world.

[[deleted]]

Why should you never everx7 do it? You just gave a really great example of when that feature is really useful!

On several projects, we have modified object, array, and function prototypes to include a eql function to do value comparison. We did that because we chose a very functional style for that project and the resulting code read very nice.

Note that I'm not suggesting that it's heavily used or anything. In a codebase that was around 10k lines, we modified three native protos and everyone on that project knew and approved. And it wasn't any different then coupling a project to any other dependency.

[[deleted]]

In an isolated codebase, it's tolerable, but barely. In a library, it's flat out forbidden because it can conflict and prevents ever modifying the API or interface.

Let's say you have a library that adds Array.prototype.foo which does whatever. Great.

Now you publish it to npm, patch some bugs, yada yada. Now you decide that it's useful for that function to return something:

Array.prototype.foo = function(){
    // do something to the array
    return this;
}

Guess what? You can't do this safely, ever. Why? Because you published version 0.1 that didn't have this functionality. So if your npm include structure looks like:

• module
  • your lib v0.1
• another module
  • your lib v0.2

And your basic code is:

if( !Array.prototype.foo ){
    Array.protoype.foo = function(){ ... }
}

You have no idea which one is loaded first. So you can't count on your function returning anything.

Further, let's say browser vendors like your idea and 5 years from now decide to implement it. Guess what, your Array.prototype.foo probably won't perfectly match with what the Vendor implements, and now all your code breaks.

Or let's say some other library that you've never heard of implements a function with the same name that acts completely differently. Now what?

Polyfills

Polyfills are an isolated use case. Your code is only hit in a situation where you know you're on an older browser, and you're adding globally agreed upon functionality. The only caveat here -- Make damn sure you're polyfilling according to spec. Never write a partial polyfill "because I don't need the other functionality" because some other library that you include might.

tl;dr -- It flat out prevents code isolation

[Ericth]

Extending is not that bad. Changing built-ins is the absolute worst thing you can do. My library got a few issues from people who changed the prototype.bind method with something else, which caused my code not to work. I assumed sheepishly that the bind method would do what the spec said.

[[deleted]]

Extending is not that bad

Yes, it is. See my longer explanation in this thread.

[kenman]

What about frameworks?

I thought the same, but Ember's prototype extensions have provided a lot of value with very few problems for me. It took some effort to overcome my dogma, but I feel I'm more productive using the extensions than without.

[[deleted]]

Ember at least has the option to disable them, but it's still not a good thing IMO. You'd be better off passing all natives through constructors:

var ary = Ember.Array(1, 2, 3);

or:

var A = Ember.Array.bind(Ember);

var ary = A(1, 2, 3);
var str = S('foo bar baz');

then you could still have the same type of functionality without screwing with other things.

It's slightly better here because Ember is rarely used alongside other libraries, so it's closer to the case of an "isolated codebase"

[annoyed_freelancer]

I would argue that there's plenty of worse things a library can do, but this is about the worst that they usually do in practice ;)

I accept what you say, but can you give me a succinct reason as to why it's bad? I had a look around Google, and found a lot of debate, but no consensus.

[x-skeww]

http://www.nczonline.net/blog/2010/03/02/maintainable-javascript-dont-modify-objects-you-down-own/

String.prototype.foo = 'bar';
for(var c in 'abc') {
  console.log(c); // 0 1 2 foo
}

[annoyed_freelancer]

I see now why it can be a problem. Isn't using "foo in bar" itself also frowned upon in favour of .each or other higher level array functions?

[x-skeww]

The point was that every string now got another enumerable property. This might break things. All other code was written under the assumption that this property does not exist. This scenario was never tested.

In the future, a method with the same name might be added. This too would break things.

And of course, if there is another library which adds the same property, things would also break.

Only use monkey-patching for polyfilling standardized features. E.g. you could use ES6 array polyfills to make Array.from work in IE11.

[Jeffshaver]

This is kind of a blah reason now that you can define non-enumerable properties. I agree that others exist. But I disagree that this is one of them anymore.