A tricky issue

[ghost]

This one is a tricky issue.. As in REACT some attributes need to be set as an property, so there should be an whitelist on this in case people set an attribute and it has to be an property.

Anyway. The issue is if you set example checked as an attribute - it should be an property. So say we now set this as an property, and still in your code this attribute - now property - will be unset as an attribute.

Look here: https://github.com/crysalead-js/dom-layer/blob/master/dist/dom-layer.js#L58-L61

So even if this now are set as an property, it can be removed as an attribute and it will. And then something will go stright to h...

I checked how REACT, Mithril, CitoJS and others solve this issue, and you can take a look at CitoJS as it's a raw copy & paste of Mithril on this point: https://github.com/joelrich/citojs/blob/master/dist/cito.js#L629-L633

You see what is done? The attributes that has to be set as an property, are set to not be removed as an attribute. Same does REACT.

You also see the issue here with class and className ? The first one can be set as an attribute, and the last one both as an attribute and className. But in your code, React and others className should only be an property.

Meaning. You need to zero out node.className also when you remove the class attribute. Else it can be an conflict.

This is all about avoiding conflicts. So what you need to do, is to avoid attributes set as property to be removed as an attribute. And vice versa.

This is also taken care of in Virtual DOM as you pointed to earlier.

And if you look into the setter part of the attributes in CitoJS - also copied from Mithril - you will see that this attributes are forced to be set as an property, just as in REACT as you pointed out to me earlier:

https://github.com/joelrich/citojs/blob/master/dist/cito.js#L608-L612

[ghost]

@jails This is just a simple solution I guess on this one, and can be done like this:

Change

for (name in previous) {
    if (previous[name] && !attrs[name]) {
      element.removeAttribute(name);
    }
  }

to this:

var inputProperties = {
    input: true,
    textarea: true,
    select: true,
    option: true
};

for (name in previous) {
    if (previous[name] && !attrs[name]) {
    // This attributes has to be removed as an property
    if (inputProperties[element.tagName.toLowerCase()] && name === "value" || name == "selectedIndex" || name === "checked"))  {
  element[name] = undefined;
} else {
      element.removeAttribute(name);
   }
    }
  }

tagName used here is the same tagName you use when you create a new Tag()

[jails]

Well the Mithril/CitoJS "patch" doesn't seem complete and afaik selectedIndex was never an attribute. Furthermore value in dom-layer should be set in attrs (indeed if the multiple property of a select multiple is set after the value it won't work (indeed it's not possible to set an array of values to a default select element which accept only one value) so the workaround was to manage value as an attrs).

Personnaly I only take care of checked, multiple, readOnly, selected in my "hyperscript" function. If they are present in attrs I'm just moving them to props with a true value, it's the only edge case I saw here. Btw I'm not sure we should repeat DOM inconsitencies in VD implementations. I would prefer make sure people are setting props && attrs accroding to a "simple logic" instead of adding further "crappy" code to deal with all the DOM mess (like for managing dataset).

Maybe we can simply filtering out attributes which should be set as a property (same for properties) like in https://github.com/crysalead-js/dom-layer/commit/92ae0be3c624b4874af5019bc0f1a55100ed2d03 ? And so throwing an error to warn the developper how props && attrs should be set ?

[ghost]

Hi. This is all your code, and your choises and opinions. I have only played a little "dumb" and helped you out with bug tracking as in fact is my special field. But if you ask my personal opinion about this issues and others? welll....

The main goal is to have a clean, readable source code, and quickly take care of edge cases if they show up. And so far, as I see it, you have messed up the code a little already not only with dataset.

Let us say the end-devs gives the opertunity to handle their own props and attrs, as well? Then you can simplify, and make code readable.

Example:

// in a separate file, you have a attributeHook
var attributeHook = {};

["checked", "multiple"].forEach( function(name) {
attributeHook[name] = function(element, key, value) {  
  element[name] = value; // Attributes that should be set as property taken care of
}
});

// In the attribute setter you now only need this
var HOOK = attributeHook [name];
if( HOOK) {
HOOK( element, name, value);  // done in the hook
} else {
// your original code goes here...
}

If this is done for both properties and attributes, the code get cleaner and easier. And all attributes, and even the value stuff can be moved to this hook.

And if you hook this hooks on the domLayer namespace, end-devs can extend both this hooks as they want if there are edge cases they need to solve there and then.

For throwing an error, return true etc. will only confuse is my opinion. Easiest would be to take care of this edge cases in the background, and auto-set attributes as properties and vice versa if needed as I show here. And then only mention in documentation later on that this will happen so the end-devs know about it.

And if there are coming more edge case attributes / properties in the future, all you need to do, is adding it to the forEach array. Simple and quick fix without messing around.

For the dataset, with the solution I come up with here, you can put it as well a hook and keep the code clean.

[ghost]

The code solution you linked too, have several issues.

1. Almost the same code used twice. codeClimate and others will scream about it.
2. A throw here would slow things down, and can hang the execution of the rest of the script if many of the same errors produced at the same time. You should at least put an error in an timeOut to prevent this.

Illustrated example:

     try {
      // code goes here
        } catch (err) {
// Will not hang the script
           window.setTimeout(function()  { throw err }, 1);
            return false;
        }

Updated To totaly avoid killing the script with an throw, just use WARN and have a look at REACT warn function.

[jails]

The hook system is interesting. I did a refactoring here https://github.com/crysalead-js/dom-layer/commit/bc38b034827923722ddaca4224e5ce50d13d4523

However I remain convinced that auto-setting attributes as properties and vice versa is not a good idea. Mainly because of a separation of concern. Then it will lead people to use indifferently props & attrs to put their "attributes" since it will work. So they can reach the same issue where an "attribute" will be set as both props & attrs so the one in attrs will overwrite the one in props. Also, attributes value and properties value are sometimes different. For example the checked is a boolean as a property but is the "checked" string as an attribute. It will probably works since "checked" will be casted as a boolean string when setted as a property but I'm not sure setAttribute("checked", true) will work on all browsers since the spec assume setAttribute("checked", "checked").

Anyhow which this "hook system" the strategy will be up to the developper.

Ps: And for the error, the dom-layer is a low level library so it shouldn't take care of error management imo. If a dev perfer to ignore errors in prod, he should use your snippet upstream in its own code.

[jails]

However if we make sure value is the last processed we probably won't need to take care of checked, multiple, readOnly and selected anymore.

[ghost]

I saw your refactoring, and the code looks similar to something I have seen before...

Regarding javascript setAttribute("checked", true), it can be set as javascript setAttribute("checked", "") for attributes and that should be equal, or just remove the attribute if the value is false.

Study REACT's code regarding this. They have a 71% good solution. Not 100% optimal because of the low performance.

How are you going to make sure value is the last processed? Such operation need to at least to be optimized and you would need a shadow object / array? Or what you think?

First that stroke my mine was check if the value exist in the object, if so copy value over to an shadow array / object, delete form original object, do the iteration and run value? But need to be carefull because delete is expensive and v8 will scream sometimes when you use this solution and ask you to unset the object value and not delete.

I see you are using delete in your code, but unset the value is most optimal, or?

[jails]

Maybe something like that: https://github.com/crysalead-js/dom-layer/commit/b5f672a76d92dcf690176ac57e5cb232fee0256d

[ghost]

Something like that is a good idea, but there are things I didn't understand. You first check if the attribute exist in the .set... , then inside the hook you call the same function hook twice to check if it's existing inside the same hook? Isn't easier to say if it exist in the hook in the first place, set it ?

[jails]

hmm which part of the code you are talking about ?

[ghost]

You almost fixed the complexity I was thinking of in the typo fix commit. That section.

[jails]

applyProps.set is a "customizable" setter function. When called the if (applyProps.set[name]) checks if a "custom" setter has been set. If so, the custom function is executed otherwise it defaults to the default behavior. Btw after thinking about it multiple seems the only attribute which require to be set before value, so maybe we can take care of it directly https://github.com/crysalead-js/dom-layer/commit/c129af64b9fde43a30765e95f1dcb6df491a69a3 (multiple will be executed twice for multiple selects but the code will remain clean for the rest)

[ghost]

Last change you did I guess is the best one, but I got a feeling telling me that something could have been done different... So need to think a little.

[ghost]

Execution twice is one CPU cycle too much, or? I have to run away and do private things now, but what about this? You create an priority?

var priority= { multiple:1, foo:2, bar:3   }
//... then

if(priority[name]) { // only attributes with priority will be executed once and only once

} else {} // the rest follows here...

This will give no duplication of code, no double execution. and if you in the future discover an attribute that need to be set before others again - like multiple. You can do it easy.

Easy now to say that multiple are the only one, but we don't know the future...

[ghost]

There are a few more catches here to study, but I need to check your code again later AND think about it and do some analyzes.

I'm not here now, and even if I prefer to be a bug finder, I can try to reply in discussions as I did in this thread if you want to discuss something. The real truth is that I have been in and out in this game the last 30 years.

[jails]

It will requires to iterate attrs & props has many times has there is different level of priority (so at least twice every time for every node). The other option is to use delete once multiple is applied but afaik the DOM is enough smart to bail out when the value you attempt to set has already been setted (so imo it would be faster that using delete). Right now it seems that's only a issue related to select multiple. And since I didn't see any other "precedence issue" in any other VD implementations, let's leave it for now and reconsider it if more edge cases come.

[ghost]

Quick reply to this. Have a closer look at REACT v. 0.3 from 2013 in the folder domUtils to get a clearer picture of all this. Readable code there comparing with today.

Now I'm going

[jails]

I'm closing this, should be fixed in https://github.com/crysalead-js/dom-layer/commit/2e7f8bc3c2399de1314bcfb78a4f581c0862f33b

[ghost]

I was studying the code, and it seems much better now, but why not add a hook for styles too as you do for the others, and expose?

One thing I was wondering about, is in the attribute setter. You are using apply, and in the style setter you are dealing with the arguments - argument[1] etc.

Any reason for this? apply are terrible slow - https://jsperf.com/function-calls-direct-vs-apply-vs-call-vs-bind/6

[jails]

Adding hook for style will add an extra overhead and I didn't see any interesting use case since applying style is pretty much straighforward. Btw if people want to deal with their own styling method it still possible to override the attrs.style "hook" https://github.com/crysalead-js/dom-layer/blob/master/node/util/attrs.js#L76-78

If you are talking about this apply https://github.com/crysalead-js/dom-layer/blob/master/node/util/attrs.js#L81 it's just the name of a function inside an object (i.e it's not the function apply() method). Btw if you have a better naming in mind it would be less confusing.

And argument[1] was probably the result of a copy paste of some code at some point https://github.com/crysalead-js/dom-element-css/commit/f105c94855b3c1469003288cb108bbf31bfbc72c.

[ghost]

I was mention this when I talked about apply: https://github.com/crysalead-js/dom-layer/blob/master/node/util/attrs.js#L77

You are using the same for dataset.

Better word then apply? I would say "parse" . Example "parseAttributes" | "parseProperties" | "parseNS" etc.

[jails]

The one you are mentioning is still a function called apply: https://github.com/crysalead-js/dom-layer/blob/master/node/util/style.js#L35

Well I'm not sure it can be considered as some parsing. Parsing means more processing something to extract some stuff. The goal of this function is to apply some new states according some older states so the element attributes/properties states match the new states.

[jails]

maybe patch would be more appropriate.

[ghost]

I'm confused too! https://github.com/crysalead-js/dom-layer/blob/master/node/util/attrs.js#L77

This looks just like a normal apply to me, so I say use patch or assign, utilize or handle

[jails]

done in https://github.com/crysalead-js/dom-layer/commit/0f74179d2c21ec7fe042e1e405e3559e835adcba

[ghost]

Ok. This has nothing to do with a confusing name, but just have a look at this code to grab some ideas maybe: https://github.com/Matt-Esch/virtual-dom/blob/master/vdom/apply-properties.js

[jails]

It's not really the same kind of hook https://github.com/Matt-Esch/virtual-dom/blob/master/docs/hooks.md and I didn't find the virutal dom hook implementation really useful.

[ghost]

When I figure out what it was, yes not the same and not really usefull. Anyway, I did some research and boolean attributes / properties can maybe cause issuses.

This is from reactiveJS - see how they do it with boolean attributes.

NOTE! There are mentioned more edge cases in this code snippet - file, editcontentiabe etc.

And also study how they did it with select multiple

function Attribute$updateEverythingElse() {
    var _ref = this;

    var node = _ref.node;
    var namespace = _ref.namespace;
    var name = _ref.name;
    var value = _ref.value;
    var fragment = _ref.fragment;

    if (namespace) {
        node.setAttributeNS(namespace, name, (fragment || value).toString());
    } else if (!this.isBoolean) {
        if (value == null) {
            node.removeAttribute(name);
        } else {
            node.setAttribute(name, (fragment || value).toString());
        }
    }

    // Boolean attributes - truthy becomes '', falsy means 'remove attribute'
    else {
        if (value) {
            node.setAttribute(name, "");
        } else {
            node.removeAttribute(name);
        }
    }
  }

 function Attribute$update() {
    var _ref = this;

    var name = _ref.name;
    var element = _ref.element;
    var node = _ref.node;var type;var updateMethod;

    if (name === "id") {
        updateMethod = updateIdAttribute;
    } else if (name === "value") {
        // special case - selects
        if (element.name === "select" && name === "value") {
            updateMethod = element.getAttribute("multiple") ? updateMultipleSelectValue : updateSelectValue;
        } else if (element.name === "textarea") {
            updateMethod = updateValue;
        }

        // special case - contenteditable
        else if (element.getAttribute("contenteditable") != null) {
            updateMethod = updateContentEditableValue;
        }

        // special case - <input>
        else if (element.name === "input") {
            type = element.getAttribute("type");

            // type='file' value='{{fileList}}'>
            if (type === "file") {
                updateMethod = noop; // read-only
            }

            // type='radio' name='{{twoway}}'
            else if (type === "radio" && element.binding && element.binding.name === "name") {
                updateMethod = updateRadioValue;
            } else {
                updateMethod = updateValue;
            }
        }
    }

    // special case - <input type='radio' name='{{twoway}}' value='foo'>
    else if (this.isTwoway && name === "name") {
        if (node.type === "radio") {
            updateMethod = updateRadioName;
        } else if (node.type === "checkbox") {
            updateMethod = updateCheckboxName;
        }
    }

    // special case - style attributes in Internet Exploder
    else if (name === "style" && node.style.setAttribute) {
        updateMethod = updateIEStyleAttribute;
    }

    // special case - class names. IE fucks things up, again
    else if (name === "class" && (!node.namespaceURI || node.namespaceURI === namespaces.html)) {
        updateMethod = updateClassName;
    } else if (this.useProperty) {
        updateMethod = updateBoolean;
    }

    if (!updateMethod) {
        updateMethod = updateEverythingElse;
    }

    this.update = updateMethod;
    this.update();
  }

[jails]

As I said a couple of days ago boolean attributes doesn't make sense imo. Only string attributes should be considered here. And in a higher abstraction level we can expect that attributes will be extracted from html templates so I see no use case to handle attributes otherwise than string values. attrs is supposed to only contain string values, further casting concern will be up ot the developper.

[ghost]

I understand that but this snippet also point out other edge cases and also ie issues. And id that should be an attr but can be set as a prop. Etc. But as you write this can be det with on a higher level.

[jails]

Since I'm not really interested to support IE <= 9, I'll let users to make the effort to open some pull requests if they want to solve a legacy browser issue. For the id I still have the same position:

• values in attrs will be set using setAttributes("id", ...)
• values id props will be set on the element directly element.id = ...

I don't get the point to not do it that way. if you want to have id to be set directly on the element, just set it in props.