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

[RC6][REGRESSION][ngUpgrade] Error: Only selectors matching element names are supported #11280

Closed
marcalj opened this issue Sep 2, 2016 · 30 comments · Fixed by #11808
Closed
Assignees

Comments

@marcalj
Copy link

marcalj commented Sep 2, 2016

I'm submitting a ... (check one with "x")

[x] bug report => search github for a similar issue or PR before submitting

Current behavior
With RC5 it's working fine.
With RC6: Error: Only selectors matching element names are supported, got: [alertManagerRow].

Reproduction of the problem
http://plnkr.co/edit/PvVWdDG5RfN7ZzVn0gUu?p=preview

Please provide a way to implement it, as you can see in the plunkr, it cannot be used as a non-attribute definition.

Please tell us about your environment:

  • Angular version: 2.0.0-rc.6
  • Browser: [all ]
  • Language: [TypeScript 2.0.2 | ES5 | SystemJS ]
@vicb
Copy link
Contributor

vicb commented Sep 5, 2016

Please describe more clearly:

  • current behavior,
  • expected behavior,
  • steps to reproduce

@marcalj
Copy link
Author

marcalj commented Sep 5, 2016

Current behaviour
With RC6 using ngUpgrade we cannot create directives like this:

@Component({
  selector: '[alertManagerRow]',
  template: `
  <td>foo</td>
  <td>bar</td>
  `
})

Use case:

<table>
        <thead>
          <tr>
            <th>1st column</th>
            <th>2nd column</th>
          </tr>
        </thead>
        <tbody>
          <tr alertManagerRow [number]="777"></tr>
          <tr alertManagerRow [number]="333"></tr>
        </tbody>
      </table>

Error: zone.js:461 Unhandled Promise rejection: Only selectors matching element names are supported, got: [alertManagerRow] ; Zone: ; Task: Promise.then ; Value: Error: Only selectors matching element names are supported, got: [alertManagerRow]

With RC5 was working perfectly.

Expected behaviour
Rendering of directive without any error. As it works without using ngUpgrade. It was working in RC5.

Steps to reproduce
Just see console error in this plunkr: http://plnkr.co/edit/ImXKSq0EOM80kVtaJVpQ?p=preview

@plgregoire
Copy link

We have the same issue on our project.
We are looking forward to have an answer on this one.
Thank you

@dions00
Copy link

dions00 commented Sep 6, 2016

We have the same issue on our project but with SVG attribute (rc.6). The application is working well with rc.5

@Component({
    selector: "[label]",
    template: `<svg:text x="0" y="15" fill="red">I love SVG!</svg:text>`
})
class LabelComponent {
}

@Component({
  selector: 'my-app',
  template: `
    <div>
      <h2>Hello {{name}}</h2>
      <svg height="30" width="200">
        <svg:g label></svg:g>
      </svg>
    </div>
  `,
})
export class App {
  constructor() {
    this.name = 'Angular2'
  }
}

Everything works fine without the UpgradeAdapter, plunk: http://plnkr.co/edit/w7Tw38KAZOlNCluliKR1?p=preview

"Only selectors matching element names are supported" error with UpgradeAdapter, just see console error in this plunk: http://plnkr.co/edit/44uWEp?p=preview

The issue is with these lines. In my example, the selector contains square brackets: [label]
https://github.com/angular/angular/blob/master/modules/@angular/upgrade/src/metadata.ts

var COMPONENT_SELECTOR = /^[\w|-]*$/;                //The brackets are not included
if (!selector.match(COMPONENT_SELECTOR)) {
    throw new Error('Only selectors matching element names are supported, got: ' + selector);
}

Thank you

@marcalj
Copy link
Author

marcalj commented Sep 7, 2016

@dions00 If I comment the line:

throw new Error('Only selectors matching element names are supported, got: ' + selector);

It's working again, great! But hope to have an official fix soon. Thanks!

@thelgevold
Copy link
Contributor

My understanding is that you can only ngUpgrade element selector based components.

I recall seeing this issue where it's explained that attribute selectors will give ownership of the host element to both frameworks: #7026

@marcalj
Copy link
Author

marcalj commented Sep 7, 2016

See my comment: #7026 (comment)

Here we're not downgrading any @Component just declaring and using it in a Angular 2 context. It works on all release candidate versions except for RC6.

@plgregoire
Copy link

plgregoire commented Sep 15, 2016

We have updated to angular 2.0.0 final and the problem described by dions00 in his plunker is still there http://plnkr.co/edit/44uWEp?p=preview . Are we doing something wrong ? Is there any workaround ?

@plgregoire
Copy link

Like dions00 has pointed out, if only the COMPONENT_SELECTOR regular expression was containing the support of the square brackets in selector, it would work like a charm.

var COMPONENT_SELECTOR = /^[\w|-]*$/; //The square brackets are not included

something like this would work
var COMPONENT_SELECTOR = /^\[*[\w|-]*\]*$/;

@plgregoire
Copy link

Looking at the code, this issue seems to have been introduced by this commit
4a740f2

                ._bootstrapModuleWithZone(
                     DynamicNgUpgradeModule, undefined, ngZone,
                     (componentFactories: ComponentFactory<any>[]) => {
                       componentFactories.forEach((componentFactory) => {
                         componentFactoryRefMap[getComponentInfo(componentFactory.componentType)
                                                    .selector] = componentFactory;
                       });
                     })

_bootstrapModuleWithZone is now called with a callback which is, according to the comment left in the code, an ugly internal api hack :)

// ugly internal api hack: generate host component factories for all declared components and
 // pass the factories into the callback - this is used by UpdateAdapter to get hold of all
 // factories.

@christopherthielen
Copy link

This is a blocker for the UI-Router hybrid app adapter past rc.5

@amcdnl
Copy link
Contributor

amcdnl commented Sep 19, 2016

@vicb - any updates on this? I can't move to latest because of ui-router not being able to. :(

@studds
Copy link

studds commented Sep 21, 2016

Trying to use ng-bootstrap also results in this issue. Even though I'm not downgrading any of the ng-bootstrap components, I get this error when trying to bootstrap using ngUpgrade:

Error: Only selectors matching element names are supported, got: [ngbDatepickerMonthView]

To be clear, I'm not using the date picker at all. Indeed, I'm not yet using any ng-boostrap component - merely to include the service.

@deepu105
Copy link

this is definitely a blocker for people using the UpdateAdapter. we are facing this with ng-bootstrap currently while trying to upgrade JHipster to ng2

@deepu105
Copy link

@vicb let us know if you need any more details

@IgorMinar
Copy link
Contributor

IgorMinar commented Sep 21, 2016

It looks like there are two issues going on:

1/ some Angular 1 applications use attribute selectors for their components and it is not possible to update these components to Angular 2 components because of a check in angular/upgrade

  • The use of attribute selector for components is possible because the definition of a component was not formalized via an API until v1.5
  • This pattern has been discouraged by Angular 1 style-guide as well as angular.component api released in v1.5

2/ some Angular 2 applications use attribute selectors for their components and when they bootstrap the application via angular/upgrade the bootstrap fails because these Angular 2 components fail to pass the same check in angular/upgrade even though they are not being downgraded to Angular 1

  • Attribute selectors should not be used for components. All of our documentation, style-guide and cli encourage element selectors to be used for components. The fact that we currently enforce this in angular/upgrade but not in angular/compiler is an oversight on our part.

These two issues while having similar symptoms are unrelated and need to be resolved separately.

I understand that some Angular 1 code might have existing components that use attribute selectors for legacy reasons. What is not clear to me and I'd really like to understand why is there a need for Angular 2 components to use attribute selectors instead of element selectors. Can someone please explain the use-cases where attribute-selectors are absolutely needed?

@amcdnl
Copy link
Contributor

amcdnl commented Sep 21, 2016

@IgorMinar -

  1. Are you are suggesting that attribute selectors are an anti-pattern?
  2. In Angular1/2, I use attribute selector for things like tooltips for instance. I want to 'enhance' the ordinary 'title' tag to have a fancy tooltip. By doing it like this it keeps the standard HTML syntax with a enhancement via angular. Additionally, there are situations where you might want to have tooltips on elements that have strict parent-child hierarchy such as SVGs. You can not wrap SVGs, so what I do is use a attribute title tag to the element, angular will then append the actual tooltip content to the body of the page and use positioning to place the tooltip appropriately.

@studds
Copy link

studds commented Sep 21, 2016

Hey @IgorMinar thanks for the prompt and thorough response.

I've got the second problem you've listed, via ng-bootstrap. I don't have an opinion as to whether attribute selectors are right or wrong. I've just started with angular 2, and it's not a great experience to have a problem with a well established library like ng-bootstrap.

Given that examples like this exist in the wild, would it be possible to temporarily relax the check in angular/upgrade and then deprecate attribute selectors for both angular/upgrade and angular/compiler?

@christopherthielen
Copy link

christopherthielen commented Sep 21, 2016

@IgorMinar thanks for looking into this issue.

UI-Router is blocked by the latter (import a module with an ng2 component with an attribute selector).

What is not clear to me and I'd really like to understand why is there a need for Angular 2 components to use attribute selectors instead of element selectors. Can someone please explain the use-cases where attribute-selectors are absolutely needed?

Attribute selectors might be necessary when the tag types are important. Something like <tr my-fancy-row> comes to mind. The <title> example by @amcdnl <title> also seems like a reasonable expectation.

The fact that we currently enforce this in angular/upgrade but not in angular/compiler is an oversight on our part.

Will that restriction be added and enforced in some future version of angular?

In UI-Router, we don't absolutely need <div ui-view='header'>. An element selector <ui-view name="header"> is fine. The assumption was that attribute selectors were supported for Components, so we added one. If the One True Way is to use only element selectors and that will be enforced at some point, I can live with that answer.

@IgorMinar
Copy link
Contributor

@amcdnl In many cases it is an anti-pattern. We are analyzing the use-cases we can find to see if all of them are anti-patterns. DOM places some restrictions on nesting of elements, but in most cases there is a solution that doesn't require attribute selectors.

Your example with the title tag is not ideal because title is a non-visual element so you can't add a tooltip to it. When it comes to tooltip itself, if you implement it as a component you will not be able to place it on another component which is likely not what you want, so if you want a tooltip that is usable in most scenarios, you should implement it as a directive. Take a look at this implementation: https://github.com/angular/material2/blob/master/src/lib/tooltip/tooltip.ts

@studds I've had a chat with @pkozlowski-opensource from ng-bootstrap about their use of attribute selectors. we'll resolve the ng-bootstrap issue one way or another.

@christopherthielen I'd strongly suggest that you switch over to element selectors for ui-view.

@pkozlowski-opensource
Copy link
Member

I definitively agree with @IgorMinar that a tooltip shouldn't be a component but rather a directive. This is what we do in ng-bootstrap as well: https://github.com/ng-bootstrap/ng-bootstrap/blob/40bde5e09d292cc714e41b1783c5bcb8e1cbd067/src/tooltip/tooltip.ts#L39

As Igor mentioned I'm going to have a look at other table-related use-cases tomorrow (tables seem to be the most restrictive when it comes to which elements you can stick between table / tbody / tr) to see what happens with other projects in the wild.

I've got the second problem you've listed, via ng-bootstrap. I don't have an opinion as to whether attribute selectors are right or wrong. I've just started with angular 2, and it's not a great experience to have a problem with a well established library like ng-bootstrap.

If we determine that attribute selector for components should be banned then we are going to change ng-bootstrap impl within a day or 2.

@amcdnl
Copy link
Contributor

amcdnl commented Sep 21, 2016

@IgorMinar -

Sorry, I misunderstood that this was restricted to components only. Please disregard my comment about the title tags.

However, you can see how I use attribute selectors on my charting framework to build svg chart components - https://github.com/swimlane/ng2d3/blob/master/src/bar-chart/bar-horizontal-stacked.component.ts#L47

@pkozlowski-opensource regarding:

I'm going to have a look at other table-related use-cases tomorrow (tables seem to be the most restrictive when it comes to which elements you can stick between table / tbody / tr) to see what happens with other projects in the wild.

SVG's are even more restrictive.

All in all, I think either way it should be slowly deprecated. Anyone upgrading their app this will just be another extra hoop for them to jump through.

@mhevery mhevery self-assigned this Sep 21, 2016
@mhevery
Copy link
Contributor

mhevery commented Sep 21, 2016

Regardless if it is antipattern or not it should not be upgrade making the check. I will have a look and should have a PR shortly.

mhevery added a commit to mhevery/angular that referenced this issue Sep 21, 2016
mhevery added a commit to mhevery/angular that referenced this issue Sep 22, 2016
@ocombe
Copy link
Contributor

ocombe commented Sep 22, 2016

Can someone please explain the use-cases where attribute-selectors are absolutely needed?

@IgorMinar for svg you cannot use any element name that you want, you often have to use <g> elements, and while selecting g[something] works, it might be nice to support just [something] in case you want to support both svg and g elements (which are both grouping elements) with this component (just an example, I'm sure there are more)

@deepu105
Copy link

Even if its an anti pattern cant it be just a warning log rather than hard
restriction? As every anti pattern will have a valid use case for some edge
cases for someone. As a global framework IMHO its not nice to have hard
restrictions on architectural patterns or use cases.

Thanks & regards,
Deepu

On 22 Sep 2016 11:17, "Olivier Combe" notifications@github.com wrote:

Can someone please explain the use-cases where attribute-selectors are
absolutely needed?
@IgorMinar https://github.com/IgorMinar for svg you cannot use any
element name that you want, you often have to use elements, and while
selecting g[something] works, it might be nice to support just [something]
in case you want to support both svg and g elements (which are both
grouping elements) with this component (just an example, I'm sure there are
more)


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11280 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlF_pkG8tvnsnNZPd5s_C7aViZTKc2ks5qshZxgaJpZM4JznJG
.

@plgregoire
Copy link

Can someone please explain the use-cases where attribute-selectors are absolutely needed?

I think @dions00 's comment demonstrate very well a use-case where attribute-selectors are absolutely needed. Like it had been said SVG must respect very strong constraints in term of hierarchy and the only way to perform data binding on a svg:rect is to use an attribute selector. If someone knows another way, I woud be happy to know it.

@plgregoire
Copy link

plgregoire commented Sep 22, 2016

@mhevery
Thank you for the fix. I tried it this morning and it works perfectly. When can we expect an angular release containing your fix ?

@IgorMinar
Copy link
Contributor

the fix is in 2.0.1

@deepu105
Copy link

Thank you it works well

Thanks & regards,
Deepu

On 24 Sep 2016 00:02, "Igor Minar" notifications@github.com wrote:

the fix is in 2.0.1


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#11280 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABDlFx4r-lIky5b8bbi2csrOm2oVYBBNks5qtBtRgaJpZM4JznJG
.

@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.