Add option: Warning on unknown variables

MatthijsZw commented 8 years ago

Sometimes we make typos on variable names (even with autosuggest). It would be great if there was a configuration so mustache-js would generate a warning on 'unknown' variables, instead of giving back an empty string (eventhough it is spec-compliant).

The Mustache man-page says: By default a variable "miss" returns an empty string. This can usually be configured in your Mustache library. The Ruby version of Mustache supports raising an exception in this situation, for instance.

phillipj commented 8 years ago

Love this feature from other frameworks while developing locally, so +1 from me! I think it's important it has minimal to no impact on performance tho. Maybe even doable while keeping the core as is? E.g. by overriding some internal methods to enable this kind of behaviour while developing.

Romanx commented 8 years ago

Maybe a mustache.dev.js which is build using mustache.js and the function overrides containing the checking logic?

ScottFreeCode commented 8 years ago

I'd prefer a hard error, so that, for instance, when using with Express it becomes a 500 response and not just a log somewhere while the end user is seeing an incorrectly rendered page (potentially very incorrectly rendered, depending on how the missing variable was supposed to be used); this could be even more useful in production than in local development, depending on how good your 500 page is vs. how bad the mis-rendered pages would be for your application's functionality. Use of sections could still allow templates to ignore missing variables even with hard errors for direct use. And any higher-level use that needs to log the issue would be in control of the logging system, so we wouldn't have to worry about, say, Mustache's internal warning mechanism clobbering output from test runners or things like that.

I have a working prototype at https://github.com/ScottFreeCode/mustache.js, although I could use some help figuring out how to write a test for it.

dasilvacontin commented 8 years ago

Hmm, so using the existence of an object as an if ({{#thing}}) would throw an error? (I think this is quite common)

Or only actual rendering of variables ({{ id }}) would raise an error? What were you thinking?

Edit: very cool feature that I would +1 to enable by default in a major in case it doesn't end up being a hassle.

MatthijsZw commented 8 years ago

In my mind the first should be an error, the second a ‘warning’. For both I would like to know there is a missing value.

Eventhough in the second case it might not technically break, it could have a huge impact on the page.

Also, the other way around would be nice: unused variables… But that’s a lot more impact to make, I think! :D

On 8 Nov 2016, at 14:19, David da Silva notifications@github.com wrote:

Hmm, so using the existence of an object as an if ({{#thing}}) would throw an error? (I think this is quite common)

Or only actual rendering of variables ({{ id }}) would raise an error? What were you thinking?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/janl/mustache.js/issues/599#issuecomment-259133973, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ8FmSptdhysYrQpg1ODkIrm12A_TXcYks5q8HbbgaJpZM4J8lKe.

dasilvacontin commented 8 years ago

In my mind the first should be an error, the second a ‘warning’. For both I would like to know there is a missing value.

@MatthijsZw I see. I just remembered you could store a null value for the property, which would be the nice thing to do, and therefore no exception would occur. (specially mentioning this for the if case)

Edit: My current position is that I'd prefer to Err on both. I prefer consistent behaviors, and you'd enforce the use of null for empty-missing values, which I think is preferable.

Also, the other way around would be nice: unused variables… But that’s a lot more impact to make, I think! :D

I'm thinking that it would be cool to somehow get the Schema for the data a template is expecting, and use that to generate a GraphQL query... or something similar.

ScottFreeCode commented 8 years ago

On further reflection, I think part of the issue here is that missing data is invalid because -- and therefore only if -- the template is expecting that data. So, missing data that the template would just plain display is always invalid by that logic, but it's conceivable that a template might in one place expect a certain data to be available and branch on it as some kind of true/false flag (so it's invalid if it's not so much false as missing and therefore not meeting that expectation), but in another it might expect that data may or may not be available and branch on whether it is (in which case it's never invalid).

In that perspective, using null to control this doesn't really make much sense to me:

If the data's wrong -- not meeting expectations -- how can we expect the wrong data to control template behavior about handling wrong data?
The expectation's really in the template anyway.
null could be a falsey value for branches on truthiness that expect data and consider it invalid for the data to be missing, but that still leaves us with a need for branching on precisely whether the data was supplied at all because we expect that the data might not be filled in (not even with null, unless it's coming from a source that uses null for missing data, e.g. SQL -- and actually, that suggests that whether to treat null as missing or not should be configurable based on the data source).

What we need is two types of branches, for the different expectations on the part of the template. Which to my knowledge is unfortunate because the language-agnostic Mustache specification, while it allows for (but does not mandate) configuration of whether missing data is an error, as far as I know doesn't have anything for a different type of branch that would differ on this point. Hmm...

On the flipside, I'm currently thinking that extraneous/surplus/unused data isn't really a matter of the data being invalid, it's a matter of the template being invalid if it doesn't use that data when the application/data/model expects it to be used. That is, if some stuff is potentially useable but the template can decide whether it's relevant, then it doesn't matter whether the template prints that stuff, but if some stuff truly needs to be displayed to the user, then if the template doesn't display it that's an error. As a sort of reversal, the expectation lies outside the template (in the model?) and the invalidity of failing to meet that expectation lies in the template. Probably best to tackle that separately, I'd figure.

The above are, I suppose, strong opinions weakly held.

phillipj commented 8 years ago

IMO interpreting null values as non-existing values is wrong.

{ name: null }

that object has a name property with a falsy value, and should therefore never be seen as invalid and therefore not a reason to throw.

A more appropriate check would be to check if a requested property has been defined like we do in mustache.hasProperty().

dasilvacontin commented 8 years ago

but in another it might expect that data may or may not be available and branch on whether it is (in which case it's never invalid).

What I was trying to convey is that, if you branch depending on key X (eg {{#X}}, the provided data should have a value for key X, either a truthy or falsy value, but definitely not undefined.

null implies "yeah, I know that there's no value. I am explicitly marking that there's no value".
undefined mostly implies that key X is not even defined (if you define a key with a value of undefined you'd be better off using null). And if the key is not defined, it's either because of being 'lazy' declaring the data (eg not using null when an object ref is lacking) or because of human error (typo, slip, confusion)

So, in this case, I would see benefits in throwing an Error. (trying to branch on a key that has a value of undefined)

On the other case, trying to render either undefined or null, not sure if there's any use-case for that. Maybe Err on that one as well.

unless it's coming from a source that uses null for missing data, e.g. SQL

Afaik, Mustache philosophy is not to use the models as-is, but to generate a 'view' from them. You could add nulls around in case your provider somehow doesn't.

I'm currently thinking that extraneous/surplus/unused data isn't really a matter of the data being invalid, it's a matter of the template being invalid if it doesn't use that data when the application/data/model expects it to be used

Hmm, I think it's pretty common not to use all the provided data. I threw the idea in mainly for tooling niceties, like the GraphQL query generation I suggested.

if some stuff truly needs to be displayed to the user, then if the template doesn't display it that's an error

But who/what decides what "stuff truly needs to be displayed to the user"? The template writer I'm guessing? If you force people to use all data in the view, then you are forcing them to generate a tailored view for each template they intend to render.

MatthijsZw commented 8 years ago

As a note: my use case was a manual setup without branching.

I had ‘other people’ create the data (events) manually and in that case I had no way of verifying they filled in all the fields or made typos in the variable-names.

My template would say “{{ event.name }} on {{ event.date }}”. In that case a missing value would create a horrible page. All fields were required, so adding logic to not display {{ event.date }} wouldn’t make sense.

In this case ‘not used’ variables would be great to know as well, to double check for typos or for items they added thinking they would ‘magically’ appear on the page :)

I’m sure this use case violates the ideology of Mustache somehow, but it’s a real scenario. And it would be great for both situations (missing values + unused values) to generate a warning.

On 9 Nov 2016, at 10:53, David da Silva notifications@github.com wrote:

but in another it might expect that data may or may not be available and branch on whether it is (in which case it's never invalid).

What I was trying to convey is that, if you branch depending on key X (eg {{#X}}, the provided data should have a value for key X, either a truthy or falsy value, but definitely not undefined.

null implies "yeah, I know that there's no value. I am explicitly marking that there's no value". undefined mostly implies that key X is not even defined (if you define a key with a value of undefined you'd be better off using null). And if the key is not defined, it's either because of being 'lazy' declaring the data (eg not using null when an object ref is lacking) or because of human error (typo, slip, confusion) So, in this case, I would see benefits in throwing an Error. (trying to branch on a key that has a value of undefined)

On the other case, trying to render either undefined or null, not sure if there's any use-case for that. Maybe Err on that one as well.

unless it's coming from a source that uses null for missing data, e.g. SQL

Afaik, Mustache philosophy is not to use the models as-is, but to generate a 'view' from them. You could add nulls around in case your provider somehow doesn't.

I'm currently thinking that extraneous/surplus/unused data isn't really a matter of the data being invalid, it's a matter of the template being invalid if it doesn't use that data when the application/data/model expects it to be used

Hmm, I think it's pretty common not to use all the provided data. I threw the idea in mainly for tooling niceties, like the GraphQL query generation I suggested.

if some stuff truly needs to be displayed to the user, then if the template doesn't display it that's an error

But who/what decides what "stuff truly needs to be displayed to the user"? The template writer I'm guessing? If you force people to use all data in the view, then you are forcing them to generate a tailored view for each template they intend to render.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/janl/mustache.js/issues/599#issuecomment-259374603, or mute the thread https://github.com/notifications/unsubscribe-auth/AJ8FmcFicDWYjSqibyWac-Sqjg-iFetnks5q8ZgqgaJpZM4J8lKe.

dasilvacontin commented 8 years ago

I’m sure this use case violates the ideology of Mustache somehow, but it’s a real scenario. And it would be great for both situations (missing values + unused values) to generate a warning.

Both seem feasible. It could be useful for CI.

antoine-pous commented 7 years ago

I'd prefer seeing the parameter directly in the view for debugging.

steverukuts commented 6 years ago

My use-case is a bit different: I use Mustache to transform Terraform templates using Gulp. A missing variable can cause instances to not boot correctly, especially when substituting in free-form strings. I came up with a quick monkey-patch to get this functionality but it's hardly ideal:

var mustache = require("mustache");

var errors = [];
var lookup = mustache.Context.prototype.lookup;

mustache.Context.prototype.lookup = function(name) {
    var value = lookup.bind(this)(name);

    if (value === undefined) {
        console.error("Unknown symbol", name);
        errors.push(name);
    }

    return value;
}

var render = mustache.render;

mustache.render = function(template, view, partials) {
    var result = render.bind(this)(template, view, partials);

    if (errors.length > 0) {
        throw {message: "Unknown symbols: " + errors.join(", ")};
    }

    return result;
}

Notes:

It doesn't give you line numbers or fully qualified symbol names
It's probably totally unsafe if you're working in a multithreaded environment

However it worked well for my purposes and I'm sure someone can adapt it if required.

stefaneg commented 5 years ago

Using mustache as a templating engine in any kind of configuration management environment requires hard errors on missing variables. I have a use case similar to @steverukuts, transforming kubernetes deployment documents. Missing variables are always errors in this use case.

steverukuts commented 5 years ago

@stefaneg a couple of months after writing that in fact I discovered that Terraform supports configuration written in JSON format so now I use this instead of using Mustache. This is much better and more programmable. We've now deprecated the use of Mustache for this and it will be removed in the next revision of our deployment pipeline.

Having looked at the Kubernetes deployment documentation I see that these are YAML files. Most programming languages have libraries that can read and write YAML so I suggest you do this instead. From my experiment, I learned that when you are trying to manipulate a machine readable format, you nearly always have a better option than Mustache.

Please note: while I do not use Mustache for anything anymore, I still feel that this is a valid feature request.

stefaneg commented 5 years ago

The solution is to use handlebars, which supports the same syntax, and also has a strict option which is exactly what is needed in this usecase.

@steverukuts You are right about the manipulating machine readable format, if you have a predetermined manipulation that you need to support, and especially if you need to have some smarts built into it. For an open ended configuration tool, that becomes too inflexible very fast, hence the need for templating. Try writing a tool supporting inserting arbitrary values in arbitrary places and...you have a templating engine very soon.

phillipj commented 5 years ago

At work we've been using kontemplate for configuring Kubernetes resource the last few years. It's basically the go templating engine, with some handy functions from sprig and custom ones in that project. It has been made as a more lightweight approach than helm for example.

In relation to the discussion above; it will also explode on unknown variables.

killdash9 commented 4 years ago

The solution is to use handlebars, which supports the same syntax, and also has a strict option which is exactly what is needed in this usecase.

This issue forced me into using handlebars as well. It's too bad this isn't supported in mustache.

eliasm307 commented 1 year ago

For anyone that doesnt want to/cant move to handlebars I've made a small package to add validation to mustache

claremacrae commented 1 year ago

For anyone that doesnt want to/cant move to handlebars I've made a small package to add validation to mustache

Just to note that I tried the above with a TypeScript project. It looks really promising, but my IDE couldn't find the types to enable me to use the package... I've logged it here: https://github.com/eliasm307/mustache-validator/issues/1

claremacrae commented 1 year ago

For anyone that doesnt want to/cant move to handlebars I've made a small package to add validation to mustache

Just to note that I tried the above with a TypeScript project. It looks really promising, but my IDE couldn't find the types to enable me to use the package... I've logged it here: eliasm307/mustache-validator#1

This is now fixed, and mustache-validator is working very well for me. Thank you @eliasm307!

janl / mustache.js

Add option: Warning on unknown variables #599