Add type strength to Field and getFieldValue

[maslade]

This change makes two changes that improve typing around Field.value and the return value of getFieldValue(). This should be backwards compatible so long as users of getFieldValue<T>() were treating its output as T | undefined.

Description

1. Redefines Field as Field<T = GenericFieldValue>, where T refers to the type of the .value property in the field. The original value type is maintained as GenericFieldValue, so this change is backwards compatible.
2. Updates getFieldValue<T>(...) to use this new type parameter in Field to produce predictably typed results. Thus, getFieldValue<string> will return a string | undefined instead of any.

Motivation

#156

How Has This Been Tested?

I modified the code locally and used the following snippet in a TypeScript enabled editor to confirm that the implied type of value is string | undefined. Repeated with boolean, {}, and MyClass.

const rend: ComponentRendering = { componentName: 'name' };
const fieldName = 'name';
const value = getFieldValue<string>(rend, fieldName);

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

• I have read the Contributing guide.
• My code follows the code style of this project.
• My change requires a change to the documentation.

[aweber1]

This looks good, but just want to clarify this statement:

This should be backwards compatible so long as users of getFieldValue() were treating its output as T | undefined.

Is it a fair assumption that users are/were doing this? Or is there anything else that can be done to ensure this wouldn't be considered a breaking change?

[maslade]

@aweber1 Sorry for delay, I missed the alert.

First, that is a safe assumption. Anyone not meeting that assumption already had a bug:

// This makes no sense. <string> should be <boolean>. Probably a copy/paste oversight.
const isChecked: boolean = getFieldValue<string>(fields, 'someCheckbox');

The current implementation returns type any so TS won't have any complaints. With this PR applied it returns type string | undefined so the assignment to boolean would be produce an error.

---

Second, I discovered another bug-in-hiding that may surface from this PR:

const maybeString: string = getFieldValue<string>(fields, 'someTextField');
f(maybeString);  // f() expects a string, this is wrong

The current implementation allows this, but it shouldn't. maybeString may be undefined, so the call to f() should be more like f(maybeString || ""). With this PR applied the error is caught at code time.

---

I want to make an improvement to this PR so before moving forward. Providing a default value in the third field should change the return type from T | undefined to just T. This way const str: string = getFieldValue<string>(fields, 'someTextField', 'default value') will work without requiring a superfluous undefined check on str.

[maslade]

This last commit ensures that if you provide a default value, the return type is implicitly T, This means the second situation I described above won't come up for users providing a default value. That at least minimizes impact on upgrade.

I imported both versions into my project for a before/after:

// Current
const oldGeneric = getFieldValueOld(oldFields, 'fieldName');  // any
const oldStr = getFieldValueOld<string>(oldFields, 'fieldName');  // any
const oldStrWithDefault = getFieldValueOld<string>(oldFields, 'fieldName', 'default value');  // any

// PR #157
const newGeneric = getFieldValue(newFields, 'fieldName');  // {} | undefined
const newStr = getFieldValue<string>(newFields, 'fieldName');  // string | undefined
const newStrWithDefault = getFieldValue<string>(newFields, 'fieldName', 'default value');  // string

[evtk]

@maslade I have a question regarding this PR. I have a component rendering (Angular) containing some 'LinkField' fields. I'm not able to extract the link fields using the getFieldValue function, since it requires the extracted field to be of type 'GenericFieldValue'.

export declare function getFieldValue<T>(renderingOrFields: ComponentRendering | {
    [name: string]: Field | Item[];
}, fieldName: string): T | undefined;

Field -> defaults to GenericFieldValue which doesn't match the type of LinkField

Am I doing something of the hook here or is this simply unsupported?

[maslade]

Hey @evtk - sorry for not seeing this sooner. I hope this isn't too late to be helpful.

The map you pass in (first parameter) supports any Field type, which includes LinkField, so you should have no trouble passing it in.

The type you get back is fully determined by T, not the specific type of Field that fieldName maps to. Since that isn't known until runtime, the function treats every field it extracts as a generic Field and relies on the <T> you pass in to type the return value:

const field = fields[fieldName] as Field<T>;

So in your case, getFieldValue<string>(rendering, 'myLinkField') should do the trick.

[evtk]

hi @maslade thanks for checking in. I guess I have missed your response too :).

I can not get this fixed, based on your suggestion.

to elaborate on my specific case: the component in question receives an Input binding containing 'content'. This content is not of a type 'ComponentRendering' but contains a collection of 'ComponentFields'.

Now I want to abstract the value of the href. I should use the getFieldValue for this instead of doing something like: this.content.my_Link?.value?.href :)

Now since I don't have a rendering, but the fields from the rendering, I can't pass in a rendering, but I should be able to pass in this:

{
    [name: string]: Field | Item[];
}

as the getFieldValue suggests:

export declare function getFieldValue<T>(renderingOrFields: ComponentRendering | {
    [name: string]: Field | Item[];
}, fieldName: string): T | undefined;

so that is what I have tried in the above example