JavaScript JSONTemplate cannot be prototyped about json-template HOT 11 CLOSED

OK, it sounds like you have a real use case for this... I would accept a patch 
for
this, but I'm not sure what the exact best way is.  If anyone else has opinions 
feel
free to chime in.

Ok, sorry, I haven't had time to tweak the json-template code till now.
This patch should fix the issue.

My preference is to make expand and render part of the prototype and set 
variables
like this._options and this._program so that they can use it. Using a closure is
still an option if you're really desperate for it.

The difference is extreme privacy vs. wasted memory.
The closure does hide the program variable and options from being modified from 
the
outside. But I don't see to much purpose for that, there are thousands of other 
ways
you could break anything in JavaScript, the _ prefix is enough to say "Don't 
touch
this, if you do, whatever breaks is your fault."
The problem of course with the closure is that for each template completely new
functions are created. This wastes memory on extra functions when they could 
just be
common functions to all Template instances. It also means that you can't use the
proxy-method pattern to enhance the abilities of the existing methods. And if 
you
don't set this._options then any methods users prototype onto the Template 
don't have
the ability to look at the options used to create the template and act properly 
based
on them.

OK, this looks good basically, but are these two lines strictly necessary?

+  if(!(this instanceof Template)) // run as `Template()` instead of `new 
Template()`
+    return new Template(template_str, options);

You're supposed make a template like:

var t = Template("foo");

not

var t = new Template("foo");

Are you only protecting against the latter case?  If so I would just leave it 
out and
document it well in the examples and tests.

I pretty much learned JavaScript from Crockford's latest book, and he says he 
never
uses "new" anymore, if I recall correctly.

Yes they are necessary. To make use of prototyping new needs to be made use of.
Template() itself does not create an instance, so we check and if `this` is not 
an
instance of what we are trying to create we pass it onto an actual construction 
of
the object using new.

This effectively means that just like Error() both Template() and new 
Template() do
precisely the same thing. It's especially necessary for Template() to work, 
otherwise
only `new Template()` can be used.

So, I went to apply this patch, but I need something to write in the unit test 
to
verify it.

This got me down this rathole of trying to understand all of JavaScript's 
idioms for
inheritance, e.g. http://javascript.crockford.com/prototypal.html and
http://brehaut.net/miscellany/Development/Javascript/Crockford_vs_Base2, etc.

I think what you're initially suggesting is just to do:

jsontemplate.Template.prototype.writeToFile = function (filename) {
  ...
}

or 

jsontemplate.Template.prototype.expand = function (filename) {
  ...
}

I'm not sure I want to recommend this to people because it's a global mutation 
that
can break other modules using jsontemplate.  It's not a big deal for smaller
programs, but the implementation is also meant to be used with server side JS, 
and
thus larger programs.

So what inheritance idiom do you have in mind for people who don't want to 
directly
modify jsontemplate.Template.protoype?  Can you provide this code with the 
patch?

Crockford's solution seems to involve his special Object.create method, and it 
seems
like a bad idea to foist this on people.

I'm just saying to support prototyping the template like any other JavaScript 
class.
It's up to people whether or not they want to prototype or just create local
functions of their own.

For those wanting to inherit, something like this should theoretically work:
var JSONTemplate = (function(jsontemplate) {
function JSONTemplate(template_str, options) {
    if(!(this instanceof jsontemplate.Template))
        return new JSONTemplate(template_str, options);
    jsontemplate.Template.call(this, template_str, options);
}
JSONTemplate.prototype = new jsontemplate.Template;
return JSONTemplate;
})(jsontemplate);

The wrapping function just makes sure that nothing breaks if someone decides to
change the global jsontemplate variable to something else.
It's just the common trick of using a call/apply on a constructor passing this 
to it,
plus the same bit I noted for the actual jsontemplate to make the function work
without the new keyword.

What I was getting is that there should be a standard idiom for overriding 
methods
without mutating globals (this doesn't seem to be very standardized in 
JavaScript). 
I haven't seen this pattern you're showing there; it seems a bit complicated for
general use.

But I've just made a TODO in the code for later, and applied the patch.  The 
test in
browser_tests.py specifies what I think you want -- please verify it because we 
could
change the structure again, and I'll only go off that test to avoid breaking it.

Thanks for the patch.

Ok the test is alright. Though just for sanity's sake you might want to test it 
with
both new ... and without the new, to make sure that use with and without the 
new is
consistent. After all the current thing to do is use it without the new.

For a pattern for overriding methods without mutating globals, yes it is 
unfortunate
but that isn't something that really exists as a standard in JavaScript. 
Normally
either you MonkeyPatch the global, or you just create your own local function.

If you want, you can make that idiom easier to do with something like this in 
the
json-template.js file:
Template.newTemplateClass = function() {
  var Template = this;
  function JSONTemplate(template_str, options) {
    if(!(this instanceof Template))
        return new JSONTemplate(template_str, options);
    Template.call(this, template_str, options);
  }
  JSONTemplate.prototype = new Template;
  JSONTemplate.newTemplateClass = this.newTemplateClass;
  return JSONTemplate;
};

Then someone would just do:
var MyTemplate = jsontemplate.Template.newTemplateClass();
MyTemplate.prototype.myOwnMethod = function() { ... };
var tpl = MyTemplate('...');
tpl.myOwnMethod();

Oh right... Just so you know it's fine to just use my real name in commit 
comments
"Daniel Friesen". It's easier to refer to me by name, "Nadir Seen Fire" is my 
newer
more unique nick, but a lot of people still refer to me by dantman.

Actually the 50+ existing tests all instantiate it without 'new', so I just 
added a
case with 'new'.

I'll probably just wait for some more feedback about inheritance.  It's not 
really
specific to the template problem, so now that we're doing the standard thing, 
we can
just leave it up to the user to pick their pattern.

Yup, I was just mentioning it for sake of the prototype test.

ie: in the unlikely edge case that:

Template.prototype.myFunc = function() { return this.expand({my:'output'}); };
new Template('{my}').expand(); // works
Template('{my}').expand(); // fails

As a result of code changing for some reason making creating a template work 
but not
actually be an instance and inherit the prototype.
I guess it's just me and seeing to many of the slim possibilities.