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

Enforce - (dash) in component/directive selector to comply with Custom Element spec #5968

Closed
IgorMinar opened this Issue Dec 16, 2015 · 10 comments

Comments

Projects
None yet
10 participants
@IgorMinar
Member

IgorMinar commented Dec 16, 2015

During the kebab-case removal we kept element selectors dasherized because of custom element spec.

Component name remains dash-cased because a dash is required by the custom element spec, which we use for guidance since even after making Angular templates case-sensitive the templates remain valid html5 fragments (although with higher fidelity due to case-sensitivity that only our html parser can see).

Very few people know about the custom element spec and the guarantees the dash gives us, so I think that it would be better to enforce that all directive/component element selectors have at least a single dash in it. There should be a way to opt out via a flag in the Component/Directive metadata, but it shouldn't be on by default.

If someone is unfamiliar with the custom element spec, the benefits of adding a dash to the element name are:

  • the element becomes a custom element - the type of the DOM node is HTMLElement instead of HTMLUnknownElement
  • in case we need it, we can benefit from the :unresolved psedo-class by registering a fake element via document.registerElement
  • the spec guarantees that browsers will not introduce native elements with a dash in the name, meaning that apps won't break in the future should browsers natively implement an element that matches an Angular Component selector (e.g. <icon>)

More info about custom elements.

To make a point how error-prone the current behavior is I'm attaching a screenshot from angular.io front page that uses <todo> element - even we get this wrong from time to time.

screen shot 2015-12-16 at 3 23 11 pm

@IgorMinar

This comment has been minimized.

Show comment
Hide comment
@IgorMinar

IgorMinar Dec 16, 2015

Member

angular.io was fixed via angular/angular.io#572

Member

IgorMinar commented Dec 16, 2015

angular.io was fixed via angular/angular.io#572

@timkindberg

This comment has been minimized.

Show comment
Hide comment
@timkindberg

timkindberg Jan 18, 2016

But it's so nice to write the tagname consistent with the class 🤔. Can we still write in camel case with the dash? E.g. foo-myComponent

But it's so nice to write the tagname consistent with the class 🤔. Can we still write in camel case with the dash? E.g. foo-myComponent

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Jan 18, 2016

Contributor

the element becomes a custom element - the type of the DOM node is HTMLElement instead of HTMLUnknownElement

Is that true? I thought as long as custom elements are not actually registered as such, they stay HTMLUnknownElements if they have a dash or not. E.g. directives in A1 are also all HTMLUnknownElements and don't become CustomElements automatically.

Despite that, I totes agree on enforcing a dash in the selector of components.

Contributor

PascalPrecht commented Jan 18, 2016

the element becomes a custom element - the type of the DOM node is HTMLElement instead of HTMLUnknownElement

Is that true? I thought as long as custom elements are not actually registered as such, they stay HTMLUnknownElements if they have a dash or not. E.g. directives in A1 are also all HTMLUnknownElements and don't become CustomElements automatically.

Despite that, I totes agree on enforcing a dash in the selector of components.

@SebastianM

This comment has been minimized.

Show comment
Hide comment
@SebastianM

SebastianM Jan 18, 2016

Contributor

@PascalPrecht Created a quick example.

It is as described by Igor (tested in Chrome and Firefox):
https://jsbin.com/bequsenewu/edit?html,output

Contributor

SebastianM commented Jan 18, 2016

@PascalPrecht Created a quick example.

It is as described by Igor (tested in Chrome and Firefox):
https://jsbin.com/bequsenewu/edit?html,output

@PascalPrecht

This comment has been minimized.

Show comment
Hide comment
@PascalPrecht

PascalPrecht Jan 18, 2016

Contributor

@SebastianM Thanks man!

Contributor

PascalPrecht commented Jan 18, 2016

@SebastianM Thanks man!

@mhevery mhevery modified the milestones: beta.01, beta.02 Jan 25, 2016

@e-oz

This comment has been minimized.

Show comment
Hide comment
@e-oz

e-oz Jan 26, 2016

HTMLUnknownElement is just interface to mark 'invalid' elements. Sorry for my ignorance in this field, but what benefits HTMLElement gives over HTMLUnknownElement?

e-oz commented Jan 26, 2016

HTMLUnknownElement is just interface to mark 'invalid' elements. Sorry for my ignorance in this field, but what benefits HTMLElement gives over HTMLUnknownElement?

@deltreey

This comment has been minimized.

Show comment
Hide comment
@deltreey

deltreey Jan 27, 2016

I didn't know about the custom component spec. I'll have to major version bump a few of my libraries if this happens.

https://github.com/deltreey/angular-input-usd
https://github.com/deltreey/angular-input-usphone
https://github.com/deltreey/angular-input-digits

I'm OK with that. Just wanted to make it obvious. There are lots of tools which would be affected by this change. I only mention it because the only way I found this was by luck of someone posting it to the programming community on Google+. So there are probably lots of affected libraries whose maintainers won't be aware until the first bug report.

This would be a significant backwards compatibility break.

I didn't know about the custom component spec. I'll have to major version bump a few of my libraries if this happens.

https://github.com/deltreey/angular-input-usd
https://github.com/deltreey/angular-input-usphone
https://github.com/deltreey/angular-input-digits

I'm OK with that. Just wanted to make it obvious. There are lots of tools which would be affected by this change. I only mention it because the only way I found this was by luck of someone posting it to the programming community on Google+. So there are probably lots of affected libraries whose maintainers won't be aware until the first bug report.

This would be a significant backwards compatibility break.

@e-oz

This comment has been minimized.

Show comment
Hide comment
@e-oz

e-oz Feb 10, 2016

To answer my own question: I just found [(ngModel)] doesn't work properly with async updates if used inside of components with tag name without dash in IE 11 Win8.1 (works fine in Win10). So now I vote for this change also :)

e-oz commented Feb 10, 2016

To answer my own question: I just found [(ngModel)] doesn't work properly with async updates if used inside of components with tag name without dash in IE 11 Win8.1 (works fine in Win10). So now I vote for this change also :)

@timruffles

This comment has been minimized.

Show comment
Hide comment
@timruffles

timruffles Jun 2, 2016

Member

One side-effect of the change is that people will not longer see "X isn't a known native property" errors. Currently [someUnknownAttribute] will only throw a Can't bind to 'someUnknownAttribute' since it isn't a known native property for tags without -. At least, that's the behaviour I'm seeing at the mo with kebab-case components.

Member

timruffles commented Jun 2, 2016

One side-effect of the change is that people will not longer see "X isn't a known native property" errors. Currently [someUnknownAttribute] will only throw a Can't bind to 'someUnknownAttribute' since it isn't a known native property for tags without -. At least, that's the behaviour I'm seeing at the mo with kebab-case components.

@vicb

This comment has been minimized.

Show comment
Hide comment
@vicb

vicb Sep 25, 2016

Member

This would be a huge BC break, we probably do not want that now.

Member

vicb commented Sep 25, 2016

This would be a huge BC break, we probably do not want that now.

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