jorgebucaran / hyperapp

1kB-ish JavaScript framework for building hypertext applications
MIT License
19.07k stars 781 forks source link

element.setAttribute('onclick', ...) ??? #175

Closed crixusshen closed 7 years ago

crixusshen commented 7 years ago

Hello, In the 0.5.x of tags version, for event handling(eg. onclick),Through`else if (name[0] === "o" && name[1] === "n") { var event = name.substr(2) element.removeEventListener(event, oldValue) element.addEventListener(event, value)

    }` to deal with, now through `element.setAttribute` to deal with,In the brower, compiled code such as :

image

This logic is exposed to the HTML on whether there is any safety problems?

jorgebucaran commented 7 years ago

@crixusshen 👋 Hi, can you please format your issue using GitHub Markdown? 🙏

@rbiggs @dodekeract Do you happen to know of any security problems when exposing event handlers in this way?

FlorianWendelborn commented 7 years ago

@jbucaran Nope. If some script can access your HTML you can't expect security anyway.

That being said I don't see any reason why this code is included in the HTML though.

jorgebucaran commented 7 years ago

@dodekeract If I'm not mistaken, because we are not using addEventListener, but doing something similar to: element.setAttribute('onclick', ...) instead.

crixusshen commented 7 years ago

@jbucaran ok, This is the problem: In the 0.5.x of tags version, For the handle of event(eg. onclick),Through the following code:

else if (name[0] === "o" && name[1] === "n") {
     var event = name.substr(2)
     element.removeEventListener(event, oldValue)
     element.addEventListener(event, value)
}

But now, So to deal with the registration of event such as onclick:

element.setAttribute(name, value)

Now, Although the logic code simple, but code of html has become bloated and huge, It looks like this:

<button onclick="function (data) {
                    for (i = 0; i < hooks.onAction.length; i++) {
                        hooks.onAction[i](name, data);
                    }

                    var result = action(model, data, actions, onError);

                    if (result === null || result === undefined || typeof result.then === &quot;function&quot;) {
                        return result;
                    } else {
                        for (i = 0; i < hooks.onUpdate.length; i++) {
                            hooks.onUpdate[i](model, result, data);
                        }

                        model = merge(model, result);
                        render(model, view, name);
                    }
                }">点击改变下面文字</button>

For these two versions of the change, what is the purpose of reach ?

lukejacksonn commented 7 years ago

@crixusshen I am not sure if I have understood correctly but it seems like this is replicable only in an earlier version of hyperapp. This is not intentional and is not observed an observed behavior in later versions for sure.

What happens if you upgrade to the next stable version, or latest stable version?

crixusshen commented 7 years ago

@lukejacksonn now using is latest stable version.

I want to know why the doms registration ofonclick` not using addEventListener, but doing something similar to: element.setAttribute('onclick', ...) instead. Although their net effect is the same. But will cause the bloated HTML code.

lukejacksonn commented 7 years ago

I see what you are saying now.. in previous versions addEventListener was used, now (at least in master) only setAttribute is used. I didn't realise that change had been committed.

Was this an oversight @jbucaran and are you seeing the same thing @crixusshen is seeing (JS functions as the value of the onclick attribute)?

leeoniya commented 7 years ago

just set all on* attrs as properties: el.onclick = ....

jorgebucaran commented 7 years ago

just set all on* attrs as properties: el.onclick = ....

Yes, this is probably it, thanks @leeoniya 👌👍.