rivets icon indicating copy to clipboard operation
rivets copied to clipboard

`rv-*` implicitly setting attributes is not a good idea at all!

Open aminroosta opened this issue 8 years ago • 12 comments

Why you do have that general rv-* binding ? If someone mistype a binding name, instead of getting an error it just sets an attribute. For example if i have a binding named jqselectmenu and mistype rv-jqselecmenu then rv throws no errors, instead it just sets at attribute on the html element.

Wouldn't a binding like rv-attr-* be a better idea ?

I am using this library for two weeks now and had this problem more than 3 times, and the last one took me 1 hour to figure out! (probably i have a problem with typing too :smile:)

aminroosta avatar Dec 11 '15 13:12 aminroosta

+1 for rv-attr-*

pppdns avatar Dec 12 '15 01:12 pppdns

This is totally possible and totally easy. Which is why Rivets is so damn cool.

Here is a rv-attr-* binder for you. http://jsfiddle.net/tvx3L58L/

The important bits:

rivets.binders['attr-*'] = function(el, value) {
  var attrToSet = this.type.substring(this.type.indexOf('-')+1)

  if(value) {
    el.setAttribute(attrToSet, value);
  } else {
    el.removeAttribute(attrToSet);
  }
}

And the HTML

<div rv-attr-beer='model.itemNeedsBeer'>

This binder will add the attribute and set its value to the bound value when the bound value is truthy. It will remove the attribute entirely when the bound value if falsy.

If you wanted something to add the attribute no matter what, then set its value to the bound item. If the bound item turns false, then set the attribute to false you will want something like this:

rivets.binders['attr-*'] = function(el, value) {
    var attrToSet = this.type.substring(this.type.indexOf('-')+1)

    el.setAttribute(attrToSet, value);
}

I basically stole and modified this binder from the source code of the rv-* binder which you can find here

If you wanted to remove the rv-* binder from your implementation entirely for whatever reason. Just override it with a method that does nothing.

Please close this issue if it helps you!

Duder-onomy avatar Dec 12 '15 02:12 Duder-onomy

@Duder-onomy thank you for your code snippets. actually that is exactly what i ended up with, i wrote my own rv-attr-* and replaced rv-* to throw an exception (saying its no implemented).

I want this to be the default behavior of rivetsjs. Why? because if you mistype a binding name it is really hard to debug.(and that happens). Also when reading the html rv-attr-* clearly shows that you are setting an attribute.

What do you think?

aminroosta avatar Dec 12 '15 05:12 aminroosta

I am neither for nor against...you could say that I am forgainst it.

Lets use this issue to gauge interest.

A few things to consider. Would the rv-* go away entirely? This would break allot of apps. The way the binders currently work is that they will fall back to the rv-* binding if no other binding is found. Would the binders default to the rv-attr-*

Duder-onomy avatar Dec 13 '15 21:12 Duder-onomy

[downvote] difference between a LIBRARY (rivets) and a framework. Frameworks enforce constraints, rules, conventions, libraries leave you free to chose. Create a project on github, push it to bower/jam, and let people who want this opt-in.

"...Rivets.js is agnostic about the objects that it can subscribe to. This makes it very flexible as it can adapt to work with virtually any library or framework, but it also means that you need to tell Rivets.js how to subscribe to those objects. This is where adapters come in to play. This feature is driven by the Sightglass library..."

terrancesnyder avatar Dec 20 '15 04:12 terrancesnyder

Agreed, it seems like it makes sense to deprecate rv-* (with opt-in disabling for new projects) in favor of rv-attr-*, and have the library raise an error for an unknown binding rather than just setting an attribute.

tjcrowder avatar Jan 16 '16 13:01 tjcrowder

Like @terrancesnyder says Rivets is a library and it provides tools to customize binders. If a projects thinks it's a bad idea to have rv-* binder, it can override it.

rivets.binders['*'] = function() {
  console.warn("Unknown binder : " + this.type);
}

I think each project should have a choice, rivets should not impose it.

jccazeaux avatar Feb 11 '16 08:02 jccazeaux

I'm having trouble understanding if people think rv-* implicitly setting attributes should be deprecated, @terrancesnyder seemed to be disagreeing with the OP because he put [downvote] but then @tjcrowder said "agreed" and wants to deprecate... I'm confused!

benadamstyles avatar Feb 11 '16 14:02 benadamstyles

Even with rv-* getting deprecated, you still have a choice. You can simply override rv-* to set an attribute (if thats what you really want).

I'm not talking about choices or library vs framework here, i think this is irrelevant.

I do think rv-* is a bad idea because it makes debugging much harder. Its a common error to mistype something. and when you do, you should get an error message (like every other language or library in the planet :smile:).

rv-attr-* makes it clear that you are setting an attribute, and when you see another binding like rv-xyz then you are sure that a binding named xyz is defined.

aminroosta avatar Feb 11 '16 16:02 aminroosta

I think that the default behavior when a binder is not found should be throw an error. Leaving the choice to the developer do whatever he wants With this change the choice would still there but with a saner default

blikblum avatar Feb 11 '16 16:02 blikblum

I understand why changing rv-* to rv-attr-* could be done. But I wonder if it's a good idea to throw an error when a binder is not found. A not found binder becomes a unknown HTML attribute. This attribute could come from a typo or an external js library imported by the project. Rivets can't know, so it shouldn't throw error when it happens ?

jccazeaux avatar Feb 11 '16 19:02 jccazeaux

Having it raise an error could in theory introduce a huge backward compatibility break.

Far better (imo) is a config open whereby you can enable that functionality if you want. It defaults to false (i.e. off, so it behaves as it does now) and you turn it on if you want to raise exceptions.

stevechilds avatar May 12 '17 13:05 stevechilds