jonnyreeves / jquery-Mustache

Mustache templating plugin for jQuery
MIT License
199 stars 59 forks source link

Options.method ambiguous and dangerous #18

Open jonrmitchell opened 11 years ago

jonrmitchell commented 11 years ago

Right now the $.fn.mustache function allows you to pass in the name of the jquery method you want it to call. It defaults to append. In my particular use case I wanted it to replace rather than append. The resultant call looked like this:

$('#users').mustache('users_template', data, {method:'html'});

A couple reasons why this call rubs me the wrong way:

  1. I had to go read the source code to know what I needed to do
  2. You open yourself up to problems if you just let the user pass in method names like that. If they pass in the wrong name, they'll get either errors if jquery doesn't have that method, or (probably even worse) unknown if jquery does have that method but it's not applicable.
  3. It's extremely confusing for those that come after me that will need to maintain it, due to the fact that it says 'html'. In most contexts, html is a content type, not a function. Which means they'll have to dig into the source code too in order to figure out what that means.

Here's my suggestion: Instead of specifying the method, specify a mode, with valid values append or replace. Then the plugin can select the jquery method appropriate for the situation. It's clear (both to current and successive users), your users no longer need to know the plugin internals, and it's more error-proof.

Just a suggestion, do with it what you will. I won't be offended either way.

Although at the very least, since there are only a handful of jquery functions that would even make sense to be passed in, (append, html, and val are the only ones I could think of off the top of my head) I'd suggest white-listing the methods that could be used and falling back on your default if they pass in a bogus one.

P.S. Also, default method/mode should be a global setting, so I don't have to change it in the source code.

jonnyreeves commented 11 years ago

Hi Jonathan, thanks for taking the time to contribute to the project :+1:

I've been thinking about the points you raised and I agree that $.fn.mustache exposes the underlying implementation details which leads to it being both error-prone and non descriptive - this is something that should change before a 'stable' 1.0 release of the library.

There's a couple of approaches we could take on this one - and I would appreciate your (any anyone else with an interest) input on the design of the API.

Add More Descriptive Methods

One approach is to add specific helper methods which do exactly what they describe, namely:

$("#my-el").appendMustache("templateName" viewModel);
$("#my-el").prependMustache("templateName" viewModel);
$("#my-el").replaceMustache("templateName" viewModel);

This makes it very clear how the API should be used and what the outcome of the method call will be. I also helps the developer understand what will be returned for the next call in the jQuery chain.

A question up for debate with this approach is the name of the method. Should it be appendMustache or appendTemplate - is there a more expressive and subsistent name?

Add optional third argument, mode

Similar to the current API, (and I believe, in-line with your suggestion above) we could allow the user to provide a string which instructs jQuery-mustache what to do when rendering the template:

$("#my-el").mustache("templateName", viewModel, "append");
$("#my-el").mustache("templateName", viewModel, "prepend");
$("#my-el").mustache("templateName", viewModel, "replace");

This would replace the existing third param (which was of type object). One potential drawback to this approach the use of "magic" strings where the user must know which string performs which action (similar to how they must know which jQuery method they want to invoke with the current API).

I would then look to support an alternative invocation which accepts a hash of params (this allows for future expansion without breaking the existing API, eg:

$("#my-el").mustache({
    template: "templateName",
    data: viewModel,
    mode: "append"
});

Of course, perhaps the best option is to introduce / update both APIs?

Thanks! Jonny.

PS: I agree with the suggestion for specifying a default mode in config - I will break this out into a new issue.

jonrmitchell commented 11 years ago

While I like the sound of appendTemplate, jquery plugins standards are to keep all functionality encapsulated in one namespace. http://docs.jquery.com/Plugins/Authoring#Plugin_Methods On that same vein, appendTemplate is pretty generic and likely to collide with other templating libraries.

Looking at the other options... I don't really like the magic strings as a third parameter either.

Hmm... I guess my vote would be to keep the simple syntax just as it is:

$("#my-el").mustache("templateName", viewModel);  // no third parameter!

(Which assumes the use of an option for default mode, a la Mustache.options('default_mode','replace');)

And then, if they want to go crazy with options, also support the object syntax:

$("#my-el").mustache({
    template: "templateName",
    data: viewModel,
    mode: "append"
});

That seems to be a good balance between simplicity and power.