Soy Component render mutates dom nodes

[jbalsas]

It looks like soy components mutate dom nodes on first render. This might cause weird side-effects like lost event listeners. Take the following code:

var button = $('#buttonId');

button.on('click', function(event) { ... });

new Dropdown({
    element: '#buttonId'
}).render();

console.log(button[0] === $('#buttonId')[0]); // false -> click listener is lost

An obvious workaround is of course to make sure the code adding the listeners runs after the soy rendering, but this is not always possible, and looks like a serious limitation to integrate metal-based components...

[eduardolundgren]

Calling render phase of a component will force to recreate its nodes, it's the same way in YUI if HTML_PARSER is not used. Few options you have here: 1) Call decorate instead of render; 2) Use event delegation in the component bounding box, that way events are preserved regardless the node instance; 3) Wait for incremental dom renderer be ready.

[jbalsas]

Hey @eduardolundgren,

1. Already tried with decorate. The result is the same (afaict)
2. This would involve making blind changes in code without an apparent reason. Code that attaches the listener and code that renders the dropdown are unaware of each other...
3. This will probably mean we won't be able to push metal-components in portal until then 😢

[mairatma]

We've talked about this offline, and confirmed that this is the expected behavior. We'll work on fixing the problematic use cases on portal side. Closing this then.

[yuchi]

IMHO having listeners attached to elements before the main component framework kicks in is wrong on an awful lot of levels.

@jbalsas What actually is the case?

[jbalsas]

Hey @yuchi, not sure how to put this in here, but just for your sake I'll try in pseudo-code 😉

<liferay-ui:icon-menu>
    <%-- begin start.jsp --%>
    <ul>
    <%-- end start.jsp --%>

    for (item in items) {

        <%-- begin render item i %-->
        <button id="myButton">
        <script>$('#myButton').on...</script>
        <%-- end render item i %-->

    }

    <%-- begin end.jsp --%>
    </ul>

    <script>
        new Dropdown({...}).decorate();
    </script>
    <%-- end end.jsp --%>
</liferay-ui:icon-menu>

It's not that I don't agree with you, it's just that code might (and does) get laid out this way causing issues. Keep in mind that menu items are independent and agnostic of who and how is using them.

[yuchi]

The issue is obviously on the fact that there’s inline JS manipulating/accessing the DOM in there.

Either use a plain old synthetic event for those kind of things or delegate everything to the component.
Managing click manually is inherently wrong and error prone.

[jbalsas]

@yuchi, obviously, but I wouldn't flat out say this code is to blame.

As wrong as it may be, this code is contributed by a small module which is just being told to render an item. It's a little bit unfair to put the blame on it when we're moving the floor under its feet...

Also, the code knows nothing about being wrapped in a specific component, so the only safe delegate to do could be to the body, and as such, it will need to make sure to clean up on spa navigation.

We'll definitely try to fix this specific part of code, but that doesn't mean that we might be breaking other code out there :(

[yuchi]

It must know it will be transformed.

What about using attributes? I'm getting less and less afraid of using onclick and siblings. Those define clearly a single responder and a delegate at the same time, and they are retainable in the decoration phase without slowing it.

[jbalsas]

Not necessarily, because it might happen that it doesn't get transformed... one could switch the wrapping menu component implementation to, let's say

• A.Dropdown -> does decorate, does not mutate dom
• Bootstrap drodpown -> does nothing
• metal.Dropdown -> mutates dom

Ideally, you'd still want the independently contributed items to work under all those scenarios, agnostic of the wrapping implementation.

Granted, the end result will be the same; we need to specify which is the recommended way of writing those little pieces so things like this doesn't happen. 😉

[eduardolundgren]

@jbalsas If markup is correct metal.Dropdown will not mutate. When incremental dom support becomes ready on metal, it will be true for any matching node by the key identifier of incremental dom, it's more flexible since the markup doesn't need to be exact the same and instead just partially equal.

[yuchi]

This is where React shines. Even more in universal—client and server—mode.

// final consumer
import { IconMenu, IconMenuItem } from 'liferay-components/icon-menu';

const DLFileEntryMenu = props => (
    <IconMenu>
      <IconMenuItem label="delete" onClick={ props.onDelete } />
    </IconMenu>
);

// liferay-components/icon-menu.js

export let IconMenu => props => ( /* default impl here */ );
export let IconMenuItem => props => ( /* default impl here */ );

// we could wrap the delegate here to have pre and after hooks
export const delegate = ({ menu, item }) => {
  IconMenu = menu;
  IconMenuItem = item;
};

// another implementer

import { delegate } from 'liferay-components/icon-menu';

export default const MyIconMenu = props => ( /* my impl here */ );
export default const MyIconMenuItem = props => ( /* my impl here */ );

delegate({
  menu: MyIconMenu,
  item: MyItemMenuItem
});

[yuchi]

Also (without getting OT) what if the implementation completely changes the interaction of an object?

What if instead of using clicks it will use mouse or touch swipes?

The contract shouldn't be on events but on attributes/properties of the element. In this specific case we have href exactly for this.

[jbalsas]

@eduardolundgren, yeah, I know, it was just an example, not blaming metal here at all... we're going to get so much better with incremental dom in place

@yuchi, that's what I mean, the contract says, give me something, I'll print it. It doesn't advertise it might mutate the nodes and you shouldn't do this or that. As Eduardo points out we might be producing the wrong markup there, but the contract doesn't state "please use this markup so nodes are properly reconciled and not re-rendered" either. Again, I'm not saying it's right, just what it is... and for sure, we're going to change it 😉