`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
- Adding a check to skip the parameter if the name is
this
after https://github.com/ngxtension/ngxtension-platform/blob/3.5.2/libs/plugin/src/generators/convert-di-to-inject/generator.ts#L127 fixes the crash, but this also results in broken code that needs to be manually fixed.
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.