rpbeukes / angular-typesafe-reactive-forms-helper

Get intellisense when using Angular Reactive Forms, no more misspelled property names, refactoring Reactive Forms is back to a trivial IDE rename task.
https://www.npmjs.com/package/angular-typesafe-reactive-forms-helper
MIT License
10 stars 3 forks source link

getSafe method returns "undefined" when running tests in coverage mode #288

Open Zureka opened 2 years ago

Zureka commented 2 years ago

I've run into this failure scenario a few times while using this library in an Angular app that I've been working on. It's probably important to note that I'm using Jest as a test runner and the shallow-render package for my component tests.

In my Angular component I'll often create a getter to compute a value based on the values in the FormGroupTypeSafe.

get canSearch(): boolean {
  return this.form.getSafe(f => f.vendorId)?.value !== null;
}

Then in the test if I try to assert the value of canSearch it works as expected, until I run the tests in --coverage mode.

For some reason the value from the getSafe call always comes back as undefined while in coverage mode. To get around this I've switched to accessing the FormControl value using the controls property on the FormGroupTypeSafe instead of using the getSafe method and this seems to work more reliably in all scenarios.

// breaks in --coverage mode
get canSearch(): boolean {
  return this.form.getSafe(f => f.vendorId)?.value !== null;
}

// works in all scenarios
get canSearch(): boolean {
  return this.form.controls.vendorId.value !== null;
}

Is there any benefit to using the getSafe method over the controls property?

rpbeukes commented 2 years ago

Hi @Zureka,

My assumption is this.form.controls has no type => any, so you would not get IntelliSense when you hit . to see the variable on it.

In that case, when you rename the form property, your editor will not help you with that...(anyway, that was my experience back then)

If that is not the case, and controls is TypeSafe, then you are in luck and there is no benefit, you could use either then.

The main problem I try to solve is to rename form properties via my editor, as I had many issues when I renamed a property. I want the editor to rename and help me or at least have the TypeScript compiler shout at me where I need to fix the issues as opposed to discovering it at runtime.

Hope that helps.

I'll need to investigate your issue closely.

If you would be so kind to point me to a sample project, or just give me some instructions on how to replicate your issue, I'm happy to give it a go πŸ˜„

I hope this package will become redundant soon as the angular team is working on implementing TypeSafty Reactive Forms properly 🀞

Chat soon πŸ˜ƒ

PS: I'm a C#/React dev during the day, and I don't give angular the love it should get...I need your help, please :)

Zureka commented 2 years ago

Hello @rpbeukes,

Thank you for getting back to me. I've put together a sample project to help demonstrate the issue. You should be able to clone that repository and see the issue described by following the steps in the README.

From my experience with this package, it appears that the controls property is also typesafe and works essentially the same as the getSafe method (but with a bit less magic to derive the property name). Renaming also seems to work when using the controls property for the control that is being renamed.

image

I saw that the Angular team is working on making the ReactiveFormsModule typesafe by default, and I hope they can use this package as a reference. I work as a consultant and won't be using Angular for my next project, so I'm not in any rush to see this issue resolved. So please, take your time πŸ˜„

rpbeukes commented 2 years ago

Thanx for your help, mate πŸ˜„ Will check it out.

howiempt commented 1 year ago

I just had this issue and the problem is that the coverage tool injects code everywhere, so the conversion from the callback to string to property name that normally works no longer does.

For example (non coverage): getSafe(x => x.country) -> { return x.country; } and can be translated to property name easily (with coverage) getSafe(x => x.country) -> { f[35]++; xczxcas[45]++; return x.country; } and other gibberish variables that must be used to track method calls/coverage

It could be modified to split on ; or something but I ended up removing it for now as it basically makes the tests irrelevant/never work.