search

leonidas / transparency

Transparency is a semantic template engine for the browser. It maps JSON objects to DOM elements by id, class and data-bind attributes.
http://leonidas.github.com/transparency/
MIT License
969 stars 112 forks source link

Matching by nodeName (tag name) considered dangerous #37

Closed miohtama closed 12 years ago

miohtama commented 12 years ago

My model had a field called "img". Also in HTML code there was <img> tag.

What happened is that IE tried to render <img> tag contents as a plain text from the model directly. On other browsers this silently fails, but IE dies in a screaming death.

Since there are several potential conflicts like this when matching by nodeName it should be disabled by default.

pyykkis commented 12 years ago

I'd prefer conventions over configuration. Let's see if we can come up with decent convention. If not, i'd rather disable element matching than supporting it via config options.

Would it work in your case, if we add a bit of custom handling for img tag? That is, something similar than we have for input tags. For an example, see https://github.com/hij1nx/weld/blob/master/lib/weld.js#L418

<div class="container">
  <img />
</div>
data = {
  img: "www.example.com"
};
<div class="container">
  <img src="www.example.com" />
</div>
miohtama commented 12 years ago

If we add custom handling it should be configurable and visible. E.g. mappings which nodeNames names autofills attributes instead (Transparency.nodeNameToAttributeMappings = { img : "src" }). It become little more complex, little too magical for my taste. I think because there is potential of conflict for every HTML tag name out there, the list is pretty long and people will stumble upon them now and then...

Alternatively have whitelist of HTML tags which are autofilled and all other tags ignored. However, this would yet again make the matcher code more complex, losing some speed maybe :(

pyykkis commented 12 years ago

Thanks for the feedback.

On second thought, whitelisting autofilled HTML tags sounds good. img tag really doesn't have value to autofill and using normal directive to set the src is consistent, explicit and safe.

Regarding whitelisting and blacklisting. Whitelisting would be safer, while blacklisting would be easier (containing only img tag atm). Anyway, that's just an implementation detail.

pyykkis commented 12 years ago

I had a discussion about the issue with @shangaslammi and I'm about to change my opinion third (and hopefully) final time. Deprecating element name matching is definitely easiest to implement, won't block at least any of our use cases and actually harmonizes the API (transparency is about semantic bindings, so no reason to support anything else).

So, after all, I'm standing behind your first proposal :) (without even optional support to element name matching).

miohtama commented 12 years ago

Good that we unanimously agree :)

pyykkis commented 12 years ago

Implemented in https://github.com/leonidas/transparency/pull/43