iamguid / ngx-mf

Bind your model types to angular FormGroup type
MIT License
42 stars 2 forks source link

Can't make it work with nonNullable fields #4

Closed e-oz closed 1 year ago

e-oz commented 2 years ago
interface M {
  id: string;
}

type MF = FormModel<M>;

function getMf(): MF {
  return new FormGroup({
    id: new FormControl<string|undefined>('a', {nonNullable: true})
  });
}
error TS2322: Type 'FormGroup<{ id: FormControl<string | undefined>; }>' is not assignable to type 'FormGroup<FormModelInnerKeyofTraverse<M, "group", DefaultInferMode, { id: string | null; }>>'.
  Types of property 'controls' are incompatible.
    Type '{ id: FormControl<string | undefined>; }' is not assignable to type 'FormModelInnerKeyofTraverse<M, "group", DefaultInferMode, { id: string | null; }>'.

TS: 4.8.4

e-oz commented 2 years ago

I've tried different Infer modes.

iamguid commented 2 years ago

Hi, lets a look

1) undefined !=null in typescript it is a different types 2) always put controls type in FormGroups, like FormGroup<MF['controls']> it is better for debug type errors and you no need to describe nested FormControls types it will be inferred automatically.

It is works example:

interface M {
  id?: string | null;
}

type MF = FormModel<M, 'group', InferModeRequired & InferModeNonNullable>;

function getMf(): MF {
  return new FormGroup<MF['controls']>({
    id: new FormControl('a', { nonNullable: true }),
  });
}

also stackblitz https://stackblitz.com/edit/angular-ngx-mf-i8wxvq?file=src%2Fapp%2Fapp.component.ts

e-oz commented 2 years ago
  1. I was not saying that, I'm not that stupid.
  2. Will try.

id?: string | null;

that is not my case, the field should be either undefined or string, never null.

iamguid commented 2 years ago

id?: string | null;

It is no matter, you can write id: string

iamguid commented 1 year ago

@e-oz Now I understand what you mean, you need type with string | undefined why you can't use null instead of undefined? You can try to use InferModeFromModel like type MF = FormModel<M, 'group', InferModeFromModel>; then all will be infer from your model and if you have model with string | undefined, then you get that type in form control.

e-oz commented 1 year ago

It's not so big issue for me, but maybe just out of curiosity you'll find out the solution :)

interface DataSourceConnector {
  readonly uuId?: string;
  readonly hostType: string;
  readonly connectionUrl: string | null;
  readonly proxyUrl?: string;
  readonly username: string | null;
  readonly password: string | null;
  readonly useProxy?: boolean;
}

type DataSourceConnectorForm = FormModel<DataSourceConnector, 'group', InferModeFromModel>;

function getDataSourceConnectorForm(): DataSourceConnectorForm {
  return new FormGroup<DataSourceConnectorForm['controls']>({
    uuId: new FormControl<string | undefined>(undefined, {nonNullable: true}),
    connectionUrl: new FormControl<string | null>(null, Validators.required),
    useProxy: new FormControl<boolean | undefined>(undefined, {nonNullable: true}),
    proxyUrl: new FormControl<string | undefined>(undefined, {nonNullable: true}),
    hostType: new FormControl<string>('Oracle', {nonNullable: true, validators: Validators.required}),
    username: new FormControl<string | null>(null, Validators.required),
    password: new FormControl<string | null>(null),
  }, Validators.required);
}

Your editor will highlight the errors.

iamguid commented 1 year ago

Ok, I will check, thanks for issue.

IhorMaistrenko commented 1 year ago

Hello! First of all, thank you for the package. @iamguid did you have chance to take a look and provide a fix for this issue?

iamguid commented 1 year ago

@IgorMaystrenko Hello, thank you for the comment, I'll check it today.

iamguid commented 1 year ago

Seems it is one line fix, sorry guys for late fix. Commit with fix will be here today.

iamguid commented 1 year ago

But I need to clarify requirements. There is a test with expected behavior, and as I understand, it is what you want, yes? @e-oz @IgorMaystrenko

    it('Undefined property and optional value https://github.com/iamguid/ngx-mf/issues/4', () => {
        interface Optional {
            optionalA?: number;
            optionalB?: number;
            optionalC: number | undefined;
            optionalD?: number | undefined;
            optionalE?: number | undefined;
        }

        type OptionalForm = FormModel<Optional, null, InferModeFromModel>;

        const fb = new FormBuilder().nonNullable;

        const wf = fb.group<OptionalForm['controls']>({
            // optionalA - when field is optinal, control should be optional
            optionalB: fb.control(123), // when field is optinal, control should be optional
            optionalC: fb.control(undefined), // when undefined on value, control value should be undefinable too
            optionalD: fb.control(undefined), // when undefined on value and field is optional, control value should be undefinable too
            // optionalE - when undefined on value and field is optional, control should be optional
        });
    })
iamguid commented 1 year ago

There is a commit, you can test your cases, if you want https://github.com/iamguid/ngx-mf/commit/f172dae2efcef0c14fca4802b6bc757a623efe01

e-oz commented 1 year ago

Thanks a lot for your efforts and time!

I've tested with that commit. Good news: it does fix the issue! :) But, it raised an error for a few of my forms. But, it was easy to fix by replacing

return new FormGroup({

with

return new FormGroup<EnvironmentForm['controls']>({

So maybe it's a breaking change, and maybe this nice trick with Form['controls'] (you taught me in this issue responses) should be documented (I didn't check if it's documented already).

iamguid commented 1 year ago

@e-oz

But, it raised an error for a few of my forms.

You need to check your types when you define FormControl<string | undefined> maybe you just missed a type. Yes, better way to define your forms is use Form['controls'], because then you don't need to define FormControl types for each field in FormGroup. You can leave it just FormControl('test') without types.

iamguid commented 1 year ago

I published a new version of package, you can upgrade.

e-oz commented 1 year ago

You need to check your types when you define FormControl<string | undefined> maybe you just missed a type.

yes, it was a place where I was using <string|null> instead, so the highlight was correct.

iamguid commented 1 year ago

So maybe it's a breaking change, and maybe this nice trick with Form['controls'] (you taught me in this issue responses) should be documented (I didn't check if it's documented already).

It's already documented here https://github.com/iamguid/ngx-mf#tips-and-tricks

IhorMaistrenko commented 1 year ago

@iamguid thank you for the quick response and fix. I've checked, it works in my test project. I will continue injecting this into another project and let you know if find something. I have a bunch of different cases there, so maybe find something.