New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow mixed case events #788

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@developit
Owner

developit commented Aug 2, 2017

No description provided.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 4288e18 on event-case into e026d9e on master.

coveralls commented Aug 2, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 4288e18 on event-case into e026d9e on master.

@KeithHenry

I think this would break React style camelCase events (like onClick).

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Aug 24, 2017

Owner

@KeithHenry you're right! not sure what I was thinking there. As for other options, maybe checking for the existence of an on${x} property to signal lowercased events...

if (name.toLowerCase() in node) name = name.toLowerCase();
Owner

developit commented Aug 24, 2017

@KeithHenry you're right! not sure what I was thinking there. As for other options, maybe checking for the existence of an on${x} property to signal lowercased events...

if (name.toLowerCase() in node) name = name.toLowerCase();
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5cb4276 on event-case into 5645573 on master.

coveralls commented Aug 24, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5cb4276 on event-case into 5645573 on master.

@jeremenichelli

This comment has been minimized.

Show comment
Hide comment
@jeremenichelli

jeremenichelli Aug 25, 2017

I was thinking about this today after Rob Dodson's talk on Polymer summit. The only way I can imagine this to work is if you have a whilelist of native DOM events and bail from lowercase if the string is not found. Sad because this would increase lib size.

jeremenichelli commented Aug 25, 2017

I was thinking about this today after Rob Dodson's talk on Polymer summit. The only way I can imagine this to work is if you have a whilelist of native DOM events and bail from lowercase if the string is not found. Sad because this would increase lib size.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Aug 26, 2017

Owner

@jeremenichelli you don't think my in check would work? Seems like it'd fix some things.

Owner

developit commented Aug 26, 2017

@jeremenichelli you don't think my in check would work? Seems like it'd fix some things.

@jeremenichelli

This comment has been minimized.

Show comment
Hide comment
@jeremenichelli

jeremenichelli Aug 26, 2017

@developit can you point the line you are doing the in check? Can't find it 😟

The issue here I think is the fact that after L67 we need to somehow know if the event name needs to be normalized to lowercase, before the listener is added.

I don't know for sure how safe this approach is but checking the full event name against the body object might help, somethink like:

if (document.body.name !== 'undefined') {
  // is native event
  name = name.toLowerCase();
}

name = name.substring(2);

jeremenichelli commented Aug 26, 2017

@developit can you point the line you are doing the in check? Can't find it 😟

The issue here I think is the fact that after L67 we need to somehow know if the event name needs to be normalized to lowercase, before the listener is added.

I don't know for sure how safe this approach is but checking the full event name against the body object might help, somethink like:

if (document.body.name !== 'undefined') {
  // is native event
  name = name.toLowerCase();
}

name = name.substring(2);
@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Aug 28, 2017

Owner

@jeremenichelli - it wasn't in this PR, was just an idea I came up with. It would look very similar to what you suggested, but rather than using document.body, it would check the current Node being mutated (since different nodes support different events):

let nameLower = name.toLowerCase();
if (node[nameLower]!=undefined) {
  name = nameLower;
}
name = name.substring(2);
Owner

developit commented Aug 28, 2017

@jeremenichelli - it wasn't in this PR, was just an idea I came up with. It would look very similar to what you suggested, but rather than using document.body, it would check the current Node being mutated (since different nodes support different events):

let nameLower = name.toLowerCase();
if (node[nameLower]!=undefined) {
  name = nameLower;
}
name = name.substring(2);
@jeremenichelli

This comment has been minimized.

Show comment
Hide comment
@jeremenichelli

jeremenichelli Aug 28, 2017

Oh I get it now @developit. Let me know if there's anything I can help with, really interested in helping to push this forward 🎉

jeremenichelli commented Aug 28, 2017

Oh I get it now @developit. Let me know if there's anything I can help with, really interested in helping to push this forward 🎉

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Aug 28, 2017

Owner

Same here! I'll update the PR with the fix and we can see what bundlesize says it costs haha

Owner

developit commented Aug 28, 2017

Same here! I'll update the PR with the fix and we can see what bundlesize says it costs haha

developit added some commits Aug 28, 2017

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 716e4bf on event-case into e29de9d on master.

coveralls commented Aug 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 716e4bf on event-case into e29de9d on master.

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Aug 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5a8d256 on event-case into 7e49a91 on master.

coveralls commented Aug 28, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling 5a8d256 on event-case into 7e49a91 on master.

@jeremenichelli

This comment has been minimized.

Show comment
Hide comment
@jeremenichelli

jeremenichelli Aug 28, 2017

Looks cool, would be nice to add tests for custom elements event if current Preact's test suit allows it. If not it would be a nice future PR.

jeremenichelli commented Aug 28, 2017

Looks cool, would be nice to add tests for custom elements event if current Preact's test suit allows it. If not it would be a nice future PR.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit Aug 28, 2017

Owner

Yup. We'll need to switch the tests from running in PhantomJS to Chrome Headless, since Phantom doesn't support web components.

Owner

developit commented Aug 28, 2017

Yup. We'll need to switch the tests from running in PhantomJS to Chrome Headless, since Phantom doesn't support web components.

@lozandier

This comment has been minimized.

Show comment
Hide comment
@lozandier

lozandier May 23, 2018

@developit Was this blocked at some point since the analysis on what this change cost as far as bundlesize & switching to Puppeteer (Headless Chrome)?

lozandier commented May 23, 2018

@developit Was this blocked at some point since the analysis on what this change cost as far as bundlesize & switching to Puppeteer (Headless Chrome)?

@robdodson

This comment has been minimized.

Show comment
Hide comment
@robdodson

robdodson May 23, 2018

btw I've been meaning to write a blog post on this...

In building custom-elements-everywhere, I've seen issues in various libraries around mixed case events. I'm increasingly of the opinion that, as a best practice, event names should just be all one word, all lowercase. That's how the platform handles things today: beforeunload, pointerdown, etc. and I've only found a few exceptions (which seem like anachronisms) e.g. DOMContentLoaded.

By sticking to all lowercase, all one-word, you free up the libraries from each needing to invent their own heuristic to grok what event name you were actually trying to use.

robdodson commented May 23, 2018

btw I've been meaning to write a blog post on this...

In building custom-elements-everywhere, I've seen issues in various libraries around mixed case events. I'm increasingly of the opinion that, as a best practice, event names should just be all one word, all lowercase. That's how the platform handles things today: beforeunload, pointerdown, etc. and I've only found a few exceptions (which seem like anachronisms) e.g. DOMContentLoaded.

By sticking to all lowercase, all one-word, you free up the libraries from each needing to invent their own heuristic to grok what event name you were actually trying to use.

@developit

This comment has been minimized.

Show comment
Hide comment
@developit

developit May 23, 2018

Owner

@lozandier yup - we can't do whitelists, so detection is the only way to go here, and I'm not sure that can even work. Maybe we'll get browsers to switch those last few event types to lowercase ;)

Owner

developit commented May 23, 2018

@lozandier yup - we can't do whitelists, so detection is the only way to go here, and I'm not sure that can even work. Maybe we'll get browsers to switch those last few event types to lowercase ;)

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