Change event attributes to on[EventName] instead of data-on[eventname]

[mthadley]

It would be really nice if we did not have to prefix event attributes with data-. This is would be helpful for a number of reasons. It more closely resembles actual HTML event attributes, and would probably be more intuitive for developers coming from other frameworks.

Another benefit, this would help us when naming functions that would be used as the value for these attributes. This is a bit contrived, but let's pretend I wanted to pass an object of attributes to an element:

render() {
  const config = {
    'data-onclick': someFunc
  };

  // I could name things differently, but jsx 
  // spread syntax would not work as expected

  const onClick = config['data-onclick'];

 // ...

  return <button {...config}>{'Click Me!'}</button>;
}

// VS

render() {
  // Much cleaner
  const config = {
    onClick: someFunc
  };

  // Working without a dash makes things a lot easier
  const {onClick} = config;

 // ...

  return <button {...config}>{'Click Me!'}</button>;
}

Currently, when inspecting the result of a render, it looks like these attributes do not make it to the DOM. This would imply that they are being treated in a special way, and would also suggest that it may be possible to just do without the data- prefix now.

Thanks, and I am very curious what others think.

[mairatma]

Sure, I see the benefits and this should be doable.

[mairatma]

This is available already at metal-incremental-dom rc.43.

Just make sure to update existing data-on[eventname] use cases to use on[EventName] instead, since only the latter is now supported.

I'll update the site and the existing metal components now.

[jbalsas]

Hey guys, I don't object to this change, but I don't think we can afford it right now... unless we fork our metal versions right now and this goes into a 2.x.x branch... this is going to make support a nightmare...

We've been on a 1.0.0-rcXX basis for a while and this is a huge change. There is no way we can now update for instance the metal version in Liferay Portal for simple bug fixes without having to get into huge amounts of code updating.

As a library, I don't think we can afford this kind of version system anymore. We already have released products depending on metal, so we need to take care of this and think these kind of changes some more.

To top it off, on[eventName] was the original design and it was changed to data-on[eventName] for some reason I don't remember.. so I'm a bit scared too about this...

[eduardolundgren]

The reason is the DOM browser native on* binding, although we make sure the on* attributes will not appear in DOM.

@jbalsas, we are trying to define all those APIs before final 1.0.0, if we release 1.0.0 with data-on will be harder to change later. By any chance it can be changed on parts you used to use only on* or you see any other problem?

[jbalsas]

Hey @eduardolundgren, this kind of change it's not acceptable from 1.0.0-rcX to 1.0.0-rcX+1. If the change needs to be done, we need to take on the toll of branching a 2.0.0 and bug fixing to 1.0.0x branches in the future.

We already shipped to Liferay Portal versions GA1 and GA2 that are now completely backwards incompatible with any future version of metal-components. I don't think this is something we should be doing 😢

[mairatma]

I totally agree with you that we can't afford this version system anymore :(

I spent this week mostly trying to release a 1.0.0 version and the only thing left for that to happen was for the tests to all pass in all browsers. They're actually already passing, but there's a weird bug going on between karma and Chrome Mobile which I'm trying to fix before doing this, so I'm hoping to be able to finish this next week at most. Once that's done we'll be able to do changes like this without breaking everyone's code, which I agree is pretty bad :(

As for this causing problems, I was a bit afraid of changing this as well, but with incremental dom the problem we had before doesn't exist anymore, like Eduardo said.

I'm really really sorry this causes you problems. If it's too much trouble I can revert and we can try to release 1.0.0 with this instead.

[eduardolundgren]

@jbalsas, rc versions are not final, they are susceptible of changes like this. Of course, it's better to be avoided, although doing that after final release is much harder than during rc. After final release this won't happen, I guarantee you.

Here are the options:

1. We add a backward compatibility plan for working with old codebase.
2. We revert it, release final 1.0.0 then start a new version with those changes - this eventually will break Liferay.

Which one do you prefer? I understand it's a late change, although is important to note that we are in rc! Thanks and sorry about that.

[mairatma]

Ok, I think we've reached a conclusion that's best for everyone.

I'll revert this (sorry guys, I know some of you wanted to use it now, just wait a bit longer) and we'll add it back right before releasing 1.0.0. That way v1.0.0 will already contain this feature, but we won't risk break ing anything (like Liferay Portal).

[mairatma]

Ok, so that wasn't quite as good a conclusion as we first thought, since we need to be backwards compatible for liferay. So what we'll do instead is to support both syntaxes to avoid any problems.

[eduardolundgren]

Yep, we support both syntax and data- is deprecated as of 2.0.0. Sorry guys!

[jbalsas]

Thanks guys, I really appreciate it. ☺️

[mairatma]

Done, metal-incremental-dom version rc.44 now supports both formats.

[yuchi]

Sorry guys, I’m missing something here for sure. I see no occurence of data-on* in Liferay.

Am I wrong?

[yuchi]

Yeah, soy files did not get found by GH.

Zorrie.