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

Custom Elements Everywhere compliance #2220

Closed
porsager opened this issue Aug 30, 2018 · 8 comments
Closed

Custom Elements Everywhere compliance #2220

porsager opened this issue Aug 30, 2018 · 8 comments

Comments

@porsager
Copy link
Contributor

@porsager porsager commented Aug 30, 2018

I've made mithril ready to be added to https://custom-elements-everywhere.com here: https://github.com/porsager/custom-elements-everywhere/tree/mithril ..

It passes all basic tests, and it doesn't need much to pass all advanced tests too..

What's missing is:

  • Set arrays as property if added in attrs
  • Set objects as property if added in attrs

I think all of these should be fairly easy to add support for if we want to. The only one I'm unsure of is the first one which serializes to a comma seperated string, which someone might rely on? (breaking change).

Should we consider including these for v2? If so I could try to make a PR fixing these things.

@isiahmeadows
Copy link
Collaborator

@isiahmeadows isiahmeadows commented Aug 30, 2018

Let me address our status for each of thise:

  • Set arrays as property if added in attrs
  • Set objects as property if added in attrs

Are you referring to an own property, and not an actual getAttribute/setAttribute call? Edit: If so, that's already workable, and I think we test for that already. (Not 100% sure how much, but I believe we do test for it enough.)

  • Support kebab-case custom events in attrs
  • Support camelCase custom events in attrs
  • Support CAPSCase custom events in attrs
  • Support PascalCase custom events in attrs

~~These are already basically done, if you're referring to anything event listener related. Mithril has never specified anything other than a basic on prefix since at least as far back as v0.2 - I've leveraged this to use Bootstrap's custom event system personally for a number of projects.

If you're referring to onfoo event listener attributes or elem.onfoo properties, we don't even support that for the DOM unless you use vnode.dom directly. I recall ripping that out of v2 (needs backported) for performance reasons, and most other frameworks are similar, at least those with some degree of maturity to them.~~

Edit: test these against next, and I believe they should pass. Most of our compatibility issues were bugs that also affected normal DOM nodes.

@porsager
Copy link
Contributor Author

@porsager porsager commented Aug 30, 2018

Thanks a lot @isiahmeadows

I've found errors in my tests for the event handling, and all tests are now passing (removed above for clarity)..

So now this is only about the arrays / objects being set as properties on the element instead of attributes.

@isiahmeadows
Copy link
Collaborator

@isiahmeadows isiahmeadows commented Aug 30, 2018

Found the cause: this condition, which handles properties, specifically excludes custom elements' properties from the magic inference semantics. I don't think a special case here would take that much code, and I don't think we'd break anyone fixing that. If anything, the fact we send everything to attributes for custom elements and not normal ones would be surprising to some consumers, especially those who don't have as well-behaved elements (some have a habit of not using attributes as fully/mostly synonymous to properties).

@porsager
Copy link
Contributor Author

@porsager porsager commented Aug 30, 2018

Ah, that's great @isiahmeadows ! I'd also be surprised if anyone relied on that..

@isiahmeadows
Copy link
Collaborator

@isiahmeadows isiahmeadows commented Sep 1, 2018

FWIW, we don't document anything special for custom elements, so it'd be reasonable for a user to expect that we would at least try to use properties before methods on them, too. A user would probably read our docs, try a custom element, and be surprised to find it doesn't work how they expected.


I tried removing the check for custom elements, and found nothing changed... 😟 I'm looking to fix that in an upcoming PR (that also includes the bug fix).

@isiahmeadows isiahmeadows mentioned this issue Sep 1, 2018
9 of 11 tasks complete
@isiahmeadows
Copy link
Collaborator

@isiahmeadows isiahmeadows commented Sep 1, 2018

@porsager The attribute issues are being addressed in #2221. It was a pretty easy fix.

@barneycarroll
Copy link
Member

@barneycarroll barneycarroll commented Sep 1, 2018

@porsager can you describe the value proposition in your own words?

@isiahmeadows
Copy link
Collaborator

@isiahmeadows isiahmeadows commented Sep 1, 2018

@barneycarroll I think the value proposition should be a low bar considering the very small diff + behavioral change.

As pointed out earlier, custom element support was untested, despite having clear behavior within the source. I see the value proposition as purely philosophical: we should just treat custom elements like any other element. This is what it tests for, and we were already 99% of the way there. It seemed arbitrary to limit custom elements to only attributes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.