Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Allow mixed case events #788

wants to merge 5 commits into from

Conversation

developit
Copy link
Member

No description provided.

@coveralls
Copy link

Coverage Status

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

Copy link

@KeithHenry KeithHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@developit
Copy link
Member Author

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
Copy link

Coverage Status

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

@jeremenichelli
Copy link

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
Copy link
Member Author

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

@jeremenichelli
Copy link

@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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

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

@coveralls
Copy link

Coverage Status

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

@coveralls
Copy link

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
Copy link

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
Copy link
Member Author

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

@lozandier
Copy link

@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
Copy link

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
Copy link
Member Author

@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 ;)

@developit
Copy link
Member Author

Fixed in X.

@developit developit closed this Mar 4, 2019
@developit developit deleted the event-case branch March 4, 2019 16:44
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.

None yet

6 participants