extend String.prototype only if requested

[davidchambers]

This pull request is not yet ready to merge: the new API is not yet documented.

In short, prototype extension will become opt-in:

var format = require('string-format');
format.extend(global);

format('Hello, {}!', 'Alice');  // => 'Hello, Alice!'
'Hello, {}!'.format('Alice');  // => 'Hello, Alice!'

Or simply:

require('string-format').extend(global);

The other change is that transformers will no longer exist in a single namespace. Instead, a new create function takes a transformers map and returns a format function:

var format = require('string-format').create({
  s: function(num) { return num === 1 ? '' : 's'; }
});

This means two unrelated packages which define their own transformers can be used together without conflict.

These two changes make it possible, for the first time, to responsibly add string-format as a dependency of a library. I look forward to doing so!

I'd love to hear your thoughts on these changes, @eush77.

[eush77]

Nice changes! 👍 Code reuse is really what npm is about.

But I have some doubts about the extending API.

var format = require('string-format');
format.extend(global);

format.extend(global) doesn't actually extend global (and why would it?), it extends String. So why global here? What is the rationale behind it? format.extend(String) seems more intuitive to me.

Also, browsers' and Node's global objects have different names, and that may cause some problems for modules that try to work in both worlds (although browserify handles it automatically). String, on the other hand, is the same thing everywhere.

Finally, if String is the same everywhere, and it can't be substituted with anything else, why require it as an argument in the first place? Why won't simple format.extendString() suffice?

[davidchambers]

Thank you for your thoughtful feedback, @eush77.

format.extend(global) doesn't actually extend global (and why would it?), it extends String. So why global here? What is the rationale behind it? format.extend(String) seems more intuitive to me.

I was thinking in terms of extending an environment (likely global or window). You're right though: the word "extend" is often used to describe adding properties to an existing object, as with _.extend. Taking this line of reasoning to its logical conclusion, the function should actually take String.prototype as its argument:

require('string-format').extend(String.prototype);

Finally, if String is the same everywhere, and it can't be substituted with anything else, why require it as an argument in the first place? Why won't simple format.extendString() suffice?

This is a matter of taste. I prefer .extend(foo) to .extendFoo(), particularly if foo is the object to which a property is being added.

I'll change the argument from an environment (global or window) to an object to extend (String.prototype). How does this sit with you?

[eush77]

I'll change the argument from an environment (global or window) to an object to extend (String.prototype). How does this sit with you?

Good!

One more thing. Have you though about returning $format in format.extend?

var format = require('string-format').extend(String.prototype);

format('Hello, {}!', 'Alice');  // => 'Hello, Alice!'
'Hello, {}!'.format('Alice');  // => 'Hello, Alice!'

And, finally, format.extend can use format as $format if no transformers passed (the most common case I guess) and avoid calling create. This is far from being a performance issue, so I am not sure if it worth it.

Thank you @davidchambers for bringing in this patch, it's a very important change!

[davidchambers]

One more thing. Have you though about returning $format in format.extend?

I have a very strong opinion on this matter. Side effects should occur in as few places as possible and, importantly, should draw attention to themselves. In general, functions with side effects should return undefined to make their impurity clear. Consider the following:

var b = _.extend(a, {foo: 1, bar: 2});

This is very confusing! _.extend both mutates its first argument and returns it. a and b now reference the same object. Reading this code I would assume the author did not intend to mutate a. Here's the fix:

-var b = _.extend(a, {foo: 1, bar: 2});
+var b = _.extend({}, a, {foo: 1, bar: 2});

I have also seen this pattern many times:

a = _.extend(a, {foo: 1, bar: 2});

Here, the author assumed _.extend to be pure since it returns the result of the merge operation. The author thought this would create a new object. It does not.

If _.extend returned undefined, there would be just one way to express this:

_.extend(a, {foo: 1, bar: 2});

Since the return value is not being used, we must be calling this function for side effects.

The first example would become:

var b = {};
_.extend(b, a, {foo: 1, bar: 2});

Again, this implies that _.extend has side effects.

So to answer your question, yes, I have thought about having format.extend return $format. It ain't gonna happen. ;)

And, finally, format.extend can use format as $format if no transformers passed (the most common case I guess) and avoid calling create. This is far from being a performance issue, so I am not sure if it worth it.

Good point. This function should not be called multiple times, though, so the cost of calling create unnecessarily will be paid once at most.

Thank you @davidchambers for bringing in this patch, it's a very important change!

Thank you for your interest in this project, which has encouraged me to give it my time and attention. :)

[eush77]

What's the progress on this? Some changes are not very relevant, and the rest seems fine to me.

[davidchambers]

Thanks for following up, @eush77. Much of my free time of late has been spent contributing to Ramda and taking the Programming Languages class on Coursera (which is terrific). I'd love to wrap up this pull request and publish a new release one day this week. We'll see if it happens. :)

[davidchambers]

I've added a file which tests the examples in the README, which I've translated from CoffeeScript to JavaScript. The only task remaining is to rework the README to introduce the format function before mentioning String.prototype.