ngxtension/ngxtension-platform

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

Kiskae opened this issue · 2 comments

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:

  • Create new project ng g application ngxtension-bug
  • Add these two components to the new project:

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);
  }
}
  • run ng g ngxtension:convert-di-to-inject --project=ngxtension-bug
  • Observe that the extend.component.ts file now looks like this:
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:

  • migrating using --path isnt affected, hence my suspicion its the additional metadata available when all files are loaded at the same time.
  • For a more complex case the migration just crashes, but I'm not able to create a small reproduction of this crash. But since it also mentions this I suspect its related:

Attempted to get information from a node that was removed or forgotten.

Node text: this

angular version: 17.3.11
ngxtension version: 3.5.2

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?

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.