angular icon indicating copy to clipboard operation
angular copied to clipboard

Missing `standalone` components declared with `button[custom-button]` are not reported by the compiler

Open Lonli-Lokli opened this issue 3 years ago • 37 comments

Which @angular/* package(s) are the source of the bug?

core

Is this a regression?

No

Description

When standalone component is created with @Component({ selector: 'button[cv-button]' it will be not reported during compile time

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-standalone-uzrsmn?file=src%2Fmain.ts

Please provide the exception or error you saw

No message is displayed in Console about missing info, uncomment 59 line to make component work

Please provide the environment you discovered this bug in (run ng version)

Angular CLI: 14.0.0
Node: 14.18.2
Package Manager: yarn 3.2.0
OS: win32 x64

Angular: 14.0.0
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router, service-worker

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1400.0
@angular-devkit/build-angular   14.0.0
@angular-devkit/core            14.0.0
@angular-devkit/schematics      14.0.0
@schematics/angular             14.0.0
rxjs                            7.5.5
typescript                      4.7.3

Anything else?

No response

Lonli-Lokli avatar Jun 13 '22 09:06 Lonli-Lokli

Yes, this is the "blind spot" in our template syntax. Essentially we can't know, from the syntax, if <button cv-button> means:

  • button element with the cv-button directive;
  • button element with the cv-button attribute (ex. added for the styling purposes).

There is a longer discussion on the topic in #3425

pkozlowski-opensource avatar Jun 13 '22 10:06 pkozlowski-opensource

@pkozlowski-opensource Thanks for clarification.

I am observing similar behaviour with sugar-style directives (*ngIf for example), is it the same reason?

Example - https://stackblitz.com/edit/angular-standalone-4tmeb9?file=src%2Fmain.ts,src%2Fapp%2Fimage.component.ts Uncomment line 72 to make it work

EDIT: Also with ()-output directives Example https://stackblitz.com/edit/angular-standalone-mdcjmm?file=src%2Fmain.ts,src%2Fapp%2Fimage.component.ts Uncomment linee 96 to make it work

Lonli-Lokli avatar Jun 13 '22 10:06 Lonli-Lokli

Yes, directives matching on even handlers (ex. (foo)="doSth()") will suffer from the same problem as again - we can't say, from the syntax itself, if (foo) is meant to be an event or a directive. In short - there are situations were we are not sure if a user meant "a directive" or just an attribute / event handler.

The *ngIf situation is a bit more complex but sometimes it is equivalent to the attribute case (if you write <div *ngIf> it will de-sugar to <ng-template ngIf><div></div></ng-template>. We are, most probably, going to improve the situation here with some errors specific to the control-flow directives.

cc @AndrewKushnir

pkozlowski-opensource avatar Jun 13 '22 12:06 pkozlowski-opensource

If it's not possible for compiler, maybe it make sense to pass this to developer? Report such things always as warnings, but allow developer's customization as it's done with eslint/stylelint

preudo code from future .angularlint

"@angular-lint/directives": [
          "error",
          {
            "reportPattern": "cv-**"
          }
        ],

Lonli-Lokli avatar Jun 13 '22 12:06 Lonli-Lokli

#46146 will be in 14.1 which adds this warning for ngIf & friends.

alxhub avatar Jun 14 '22 21:06 alxhub

I was asked to copy and paste here the description of a duplicate issue I opened (#46661), so here it goes:

If a directive has an attribute selector and the attribute is used without square brackets, such as routerLink="/users", and we forget to import the directive, or the module defining the directive, then the compiler doesn't complain at all, and we end up with an unnoticed bug.

When using NgModules, such a bug happens rarely, because you rarely forget to import the router module (for example) in your main or feature module. But when using standalone components, it's much more frequent, because the import needs to be repeated on every component that needs the directive, and of course we tend to only import what's absolutely needed.

After migrating two relatively small applications to standalone components, I've introduced such a bug, especially with routerLink, at least twice.

I understand why the compiler doesn't complain, but this, IMHO, is a source of hard to detect bugs: I doubt most people have integration tests checking that each and every link in their components works as expected. And even at runtime, the bug goes unnoticed until you click on the link and realize it doesn't work. Proposed solution

It would be nice to be able to activate some compiler option that would, for example, check that any non-standard HTML attribute used in a component (such as routerLink) matches with a known directive selector for that component.

Or to be able to provide a list of selectors or attributes that must be checked.

jnizet avatar Jul 01 '22 09:07 jnizet

+💯 huge bug source... I'd love to see a strict build option that complains about any "unknown attribute". This way we are sure that imports are not missing.

Rather than maintaining a list of all known html attributes (if that is the main concern/difficulty)... at least give us a "allowedAttributes: []" where we can specify which attributes are safe to ignore (do not require an import).

joergbaier avatar Aug 31 '22 19:08 joergbaier

In short - there are situations were we are not sure if a user meant "a directive" or just an attribute / event handler.

To me this a pretty big design flaw 🤔

joergbaier avatar Aug 31 '22 19:08 joergbaier

I don't know if this is the same - I think might be another use case. We have lots of custom form control components to use inside mat-form-field. The compiler also doesn't detect these as being missed if they are in the template but not imported in the standalone component. Should I raise a new issue for this or is the underlying problem the same?

Here app-rich-text-form-control isn't imported but the compiler doesn't pick it up. Fails at runtime

  standalone: true,
  imports: [CommonModule, ...MATERIAL_COMMON],
  template: `
    <mat-form-field appearance="fill">
      <app-rich-text-form-control></app-rich-text-form-control>
    </mat-form-field>
    ...

jpduckwo avatar Dec 14 '22 00:12 jpduckwo

+1 for major pain point with standalone components

kyjus25 avatar Jun 19 '23 16:06 kyjus25

Just wanted to mention for everyone here, Webstorm (specifically the EAP version is what I tried) DOES pick up these warnings in the editor. Might save you a headache or two after you convert everything to standalone to load up the 30 day Webstorm trial and comb through your HTML files

Screenshot 2023-06-21 at 11 39 53 AM

kyjus25 avatar Jun 21 '23 16:06 kyjus25

I am also feeling sad when migrated but missing the tool support.

thanks @kyjus25 for alternatives. looking for the official angular plugin to help sort out missing directives and components.

softgandhi avatar Jul 10 '23 14:07 softgandhi

My suggestion would be to solve this with a new extended diagnosis. The new diagnosis would warns for unknown tags and unknown attributes. Each project can gradually configure the diagnosis with a list of allowed unknown tags and allowed unknown attributes. In addition users can opt-in for this rule to break the build by configuring the diagnostic as an error, to be really strict.

Of course this is not an ideal solution, but it should be relatively simple to implement. And it would be better than runtime errors, and so much better than runtime lack of functionality (as seen with routerLink).

PowerKiKi avatar Oct 17 '23 10:10 PowerKiKi

@PowerKiKi I'm fine with not ideal as long as it provides some peace of mind. Nothing scarier than deploying code that may or may not be silently broken. To me this seems like top priority

kyjus25 avatar Oct 19 '23 17:10 kyjus25

Since when though did it work for non-standalone components?

yhaiovyi-tripactions avatar Nov 09 '23 20:11 yhaiovyi-tripactions

Is there any progress on this? I tried using the angular extended diagnosis and hoped the error missingControlFlowDirective would be thrown at compile time by adding this to my tsconfig.json for cases were the import of a structural directive was forgotten.

"angularCompilerOptions": {
        ...
        "extendedDiagnostics": {
            "checks": {
                "missingControlFlowDirective": "error"
            },
            "defaultCategory": "error"
        }
    }

According to the docs this should report missing imports of structural directives as errors

This diagnostics ensures that a standalone component which uses known control flow directives (such as *ngIf, *ngFor, *ngSwitch) in a template, also imports those directives either individually or by importing the CommonModule. Alternatively, use Angular's built-in control flow.

However, this only works partially.

It works for structural directives that are applied on html elements or ng-container. But if the structural directive is applied on a custom component <app-my-component *ngIf="isAuthenticated"></app-my-component> it does not report an error

Zarlex avatar Feb 22 '24 19:02 Zarlex

@JeanMeche you participated in a related issue, but I don't think you shared your thought on the possible solution I suggested ?

Is the Angular team interested in further exploring that idea ?

PowerKiKi avatar Jun 13 '24 04:06 PowerKiKi

Maybe @nartc could bail us out with an ngxtension schematic in the mean time? 😁 👉👈

kyjus25 avatar Jun 13 '24 18:06 kyjus25

@kyjus25 what do you think the schematic should do?

nartc avatar Jun 13 '24 18:06 nartc

@nartc Ideally it would identify unimported directives and import them into standalone components. Not really what schematics are used for, I get it, but if there's any tool for the job to help until Angular is fixed it would be ngxtension 🤷

kyjus25 avatar Jun 13 '24 18:06 kyjus25

Maybe a solution https://x.com/synalx/status/1811586509277217008 ? I'll give it a try 😄

eneajaho avatar Jul 12 '24 07:07 eneajaho

@eneajaho Fantastic!!!! Thank you for your time and taking a look!

kyjus25 avatar Jul 12 '24 15:07 kyjus25

@eneajaho that would be amazing, see my relevant request here. https://github.com/angular-eslint/angular-eslint/issues/1907

It would be REALLY nice if you could also handle unused imports too https://github.com/angular/angular/issues/46766

joergbaier avatar Jul 12 '24 18:07 joergbaier

@eneajaho Sorry to bother. I pulled your https://github.com/eneajaho/angular/tree/feat/potential-directives-in-templates branch down and fumbled my way thru getting the proper test to run, but the directives array came up empty. Was this your conclusion as well or did I do something wrong? I was going to see if I could assist but from what I saw you had configured the test right but the function didn't seem to work.

kyjus25 avatar Jul 24 '24 15:07 kyjus25

@eneajaho Sorry to bother. I pulled your https://github.com/eneajaho/angular/tree/feat/potential-directives-in-templates branch down and fumbled my way thru getting the proper test to run, but the directives array came up empty. Was this your conclusion as well or did I do something wrong? I was going to see if I could assist but from what I saw you had configured the test right but the function didn't seem to work.

I came to same conclusion, but I need to check more on that.

eneajaho avatar Jul 24 '24 15:07 eneajaho