Skip to content

Fix binding event handlers using latest snabbdom#8

Open
hath995 wants to merge 2 commits into
irony:masterfrom
hath995:master
Open

Fix binding event handlers using latest snabbdom#8
hath995 wants to merge 2 commits into
irony:masterfrom
hath995:master

Conversation

@hath995

@hath995 hath995 commented Jul 9, 2020

Copy link
Copy Markdown

I was trying out pureact but element event handlers were not being bound correctly using snabbdom 0.7.4. I figured out this fix and figured I would contribute it.

@vercel

vercel Bot commented Jul 9, 2020

Copy link
Copy Markdown

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@irony

irony commented Jul 19, 2020

Copy link
Copy Markdown
Owner

Thanks a lot! I’m trying to keep the code as tiny as possible so before merging, could you figure out a neat way to do the list of names shorter? Maybe just traverse the properties on a node or look inside snabbdom for a list we can reuse?

@hath995

hath995 commented Jul 20, 2020

Copy link
Copy Markdown
Author

It certainly could be smaller to just try to detect attributes starting with "on" but it would trigger on attributes like "only-data" or "on-custom-attribute" which would result in those attributes going missing and being added as a weird event handler. I think that would add a gotcha that users would probably complain about.
This is probably the shortest I can think of an alternative but it isn't super nice to work with. The list doesn't exist in Snabbdom.

const onScriptRegex = /on(hashchange|load(ed(data|metadata)|start|)|o(ffline|nline)|p(opstate|a(ge(hide|show)|ste|use)|lay(ing|)|rogress)|unload|b(efore(print|unload)|lur)|c(hange|lick|o(ntextmenu|py)|anplay(through|)|u(t|echange))|focus|in(put|valid)|s(e(arch|lect|ek(ed|ing))|croll|t(orage|alled)|u(bmit|spend))|key(down|press|up)|mouse(down|move|o(ut|ver)|up|wheel)|d(blclick|r(ag(en(d|ter)|leave|over|start)|op)|etail|urationchange)|a(fterprint|bort)|e(rror|mptied|nded)|r(es(ize|et)|atechange)|timeupdate|volumechange|w(heel|aiting))/

function collectOns(obj) {
  let on = {};
  for(let key in obj) {
    let name = key.toLocaleLowerCase();
    if(onScriptRegex.test(name)) {
      on[name.slice(2)] = obj[key];
      delete obj[key];
    }
  }
  return on;
}

Alternatively, we could just move the list into another file and import it. It would be a bit neater. I don't think it really adds all that much to the code size and would be easier to maintain than the regex, since new events will be added in the future by the browsers.

@hath995

hath995 commented Jul 20, 2020

Copy link
Copy Markdown
Author

script to generate the regex.

var samples= require('./regex.json')
var { RadixTree } = require('xradix');
let tree= new RadixTree();
samples.forEach(x => tree.set(x))
function RadixNodeToS(node) {
    let elems = Array.from(node.c.m)
    let pstrings = elems.map(pair => [pair[0], RadixNodeToS(pair[1])]);
    return pstrings.map(pair => pair[0]+(pair[1]!=="|" ? "("+pair[1]+")" : "")).join("|") + (node.b ? "|" : "");
}
console.log(RadixNodeToS(tree.root))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants