Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(binding): save property type metadata to bindable definition #1689

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(binding): save property type metadata to bindable definition #1689

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
421d332
feat(binding): save property type metadata to bindable definition
ekzobrain Mar 1, 2023
b808027
feat(binding): add types
ekzobrain Mar 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions packages/runtime-html/src/bindable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { isString, objectFreeze, objectKeys } from './utilities';
import type { Constructable, Writable } from '@aurelia/kernel';
import type { InterceptorFunc } from '@aurelia/runtime';

type PropertyType = typeof Number | typeof String | typeof Boolean | typeof BigInt | { coercer: InterceptorFunc } | Class<unknown>;
type PropertyType = typeof Number | typeof String | typeof Boolean | typeof BigInt | typeof Array | typeof Object | typeof Function | { coercer: InterceptorFunc } | Class<unknown>;

export type PartialBindableDefinition = {
mode?: BindingMode;
Expand Down Expand Up @@ -242,16 +242,22 @@ export class BindableDefinition {
public readonly primary: boolean,
public readonly property: string,
public readonly set: InterceptorFunc,
public readonly type?: PropertyType,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two new properties should not be undefined. If there is no value available, please use null for type (and thereby making the type of type to PropertyType | null) and true for nullable (considering sensible defaults; what do you think @bigopon?).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you prefer null over undefined? You mean that undefined stands for unset value, and null - for empty value?
In case of nullable we cannot set true as a default, it should be undefined/null, because logic behind this field is: use value if it is defined, otherwise use global setting, which defaults to true. So if we set true here, global settings will neder be invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sayan751 reply to the above, please. So I will finish up with this

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you prefer null over undefined? You mean that undefined stands for unset value, and null - for empty value?

Yes. Also, undefined can indicate absence of the value or the property in the object. As far as I know, there is seldom optional properties in the internal classes used in Au2 that deals with user (devs) provided values/configuration. Plus maintaining strict type definition also means that when you are making any comparison, you can safely and strictly compare it with null; that is=== null instead of == null or other sort of comparison that results in coercion, and thereby less work at runtime.

In case of nullable we cannot set true as a default, it should be undefined/null, because logic behind this field is: use value if it is defined, otherwise use global setting, which defaults to true. So if we set true here, global settings will neder be invoked.

The property can be set to null if no value is explicitly provided. However, I am not sure what global setting you are referring to. As far as I understand, the nullable property introduced in the BindableDefinition class isn't used anywhere.

Sidenote: only an explicit nullable: false makes sense. Refer to the docs in this regard. Hence, my suggestion to set it to true by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The property can be set to null if no value is explicitly provided. However, I am not sure what global setting you are referring to. As far as I understand, the nullable property introduced in the BindableDefinition class isn't used anywhere.

I mean coercionConfiguration.coerceNullish, see line 381. It is used when nullable is null.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. I see what you have meant. I comment is regarding L260 when the value is persisted in BindableDefinition#nullable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think interceptor is kind of a public facing value, forcing it to be null will results in annoyance down the line. We can leave it at undefined. Probably applies for many other public facing stuff as well.

public readonly nullable?: boolean,
) { }

public static create(prop: string, target: Constructable<unknown>, def: PartialBindableDefinition = {}): BindableDefinition {
const type = def.type ?? Metadata.get('design:type', target, prop);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the firstDefined for consistency. And use null as fallback.


return new BindableDefinition(
firstDefined(def.attribute, kebabCase(prop)),
firstDefined(def.callback, `${prop}Changed`),
firstDefined(def.mode, BindingMode.toView),
firstDefined(def.primary, false),
firstDefined(def.property, prop),
firstDefined(def.set, getInterceptor(prop, target, def)),
firstDefined(def.set, getInterceptor(type, def.nullable)),
type,
def.nullable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the firstDefined for consistency. And use a fallback.

);
}
}
Expand Down Expand Up @@ -345,8 +351,7 @@ const Coercer = {
}
};

function getInterceptor(prop: string, target: Constructable<unknown>, def: PartialBindableDefinition = {}) {
const type: PropertyType | null = def.type ?? Metadata.get('design:type', target, prop) ?? null;
function getInterceptor(type?: PropertyType, nullable?: boolean) {
if (type == null) { return noop; }
let coercer: InterceptorFunc;
switch (type) {
Expand All @@ -367,13 +372,13 @@ function getInterceptor(prop: string, target: Constructable<unknown>, def: Parti
}
return coercer === noop
? coercer
: createCoercer(coercer, def.nullable);
: createCoercer(coercer, nullable);
}

function createCoercer<TInput, TOutput>(coercer: InterceptorFunc<TInput, TOutput>, nullable: boolean | undefined): InterceptorFunc<TInput, TOutput> {
return function (value: TInput, coercionConfiguration: ICoercionConfiguration | null): TOutput {
if (!coercionConfiguration?.enableCoercion) return value as unknown as TOutput;
return ((nullable ?? ((coercionConfiguration?.coerceNullish ?? false) ? false : true)) && value == null)
return ((nullable ?? !(coercionConfiguration?.coerceNullish ?? false)) && value == null)
? value as unknown as TOutput
: coercer(value, coercionConfiguration);
};
Expand Down