ngxtension / ngxtension-platform

Utilities for Angular
https://ngxtension.netlify.app/
MIT License
531 stars 77 forks source link

`ngxtension:convert-di-to-inject` fails to handle virtual parameters #429

Open Kiskae opened 3 weeks ago

Kiskae commented 3 weeks ago

When migrating an entire project at once, the additional metadata available to ts-morph appears to change the AST and causes erroneous changes to the code.

Steps to reproduce:

base.component.ts

import { Component, ElementRef } from '@angular/core';
import { ActivatedRoute } from '@angular/router';

@Component({
  template: ``,
  standalone: true,
  imports: [],
})
export class AppBaseComponent {
  constructor(
    readonly activatedRoute: ActivatedRoute,
    elementRef: ElementRef,
  ) {}
}

extend.component.ts

import { Component, ElementRef } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { AppBaseComponent } from './base.component';

@Component({
  template: ``,
  standalone: true,
  imports: [],
})
export class AppExtendComponent extends AppBaseComponent {
  constructor(
    readonly activatedRoute: ActivatedRoute,
    elementRef: ElementRef,
  ) {
    super(activatedRoute, elementRef);
  }
}
import { Component, ElementRef, inject } from '@angular/core';
import { ActivatedRoute } from '@angular/router';
import { AppBaseComponent } from './base.component';

@Component({
  template: ``,
  standalone: true,
  imports: [],
})
export class AppExtendComponent extends AppBaseComponent {
      public readonly this = inject(undefined);

  constructor(
    .
  ) {
        const elementRef = inject(ElementRef);
        const activatedRoute = inject(ActivatedRoute);

    super(this.this.activatedRoute, elementRef);
  }
}

Other observations:

angular version: 17.3.11 ngxtension version: 3.5.2

eneajaho commented 2 weeks ago

Thanks for opening this issue @Kiskae

Currently, what we can do to not break the whole app during migration, is to just skip the components that extend a class, and skip classes that are extended.

Edit: Later on, we can also add support for these cases too ofc.

What do you think?

Kiskae commented 2 weeks ago

Its probably hard to avoid breaking some parts of the app during this migration, especially when inheritance is involved since it changes constructors.

imho it might be an idea to run the upgrade as usual, but bail on the file and emit a warning if it finds a constructor with a this parameter.

It might be possible that the migration works just fine once the super(...) has been reduced to super(). I've not checked whether that resolves the issue.