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.
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
feat(binding): save property type metadata to bindable definition #1689
base: master
Are you sure you want to change the base?
Conversation
Looks fine to me, though I'd prefer to have @Sayan751 poke on this. Also, having no test is kind of not ok for this. |
Will add test after you approve in general |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look good to me. Only commented about some small stuffs.
Apart from that there are 2 things.
Firstly, the tests for this needs to be put in place considering how low-level critical piece of the framework it is.
Secondly, can you please elaborate a little bit more why do you need this change? I thought that for doc generation you need the typing info only during compilation (TS transpilation to AST, to be more precise). I am thinking about TypeDoc here. Furthermore, I have not used the Storybook much. Hence more details on the purpose for this change would be nice.
@@ -242,16 +242,22 @@ export class BindableDefinition { | |||
public readonly primary: boolean, | |||
public readonly property: string, | |||
public readonly set: InterceptorFunc, | |||
public readonly type?: PropertyType, |
There was a problem hiding this comment.
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?).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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 static create(prop: string, target: Constructable<unknown>, def: PartialBindableDefinition = {}): BindableDefinition { | ||
const type = def.type ?? Metadata.get('design:type', target, prop); |
There was a problem hiding this comment.
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.
firstDefined(def.set, getInterceptor(prop, target, def)), | ||
firstDefined(def.set, getInterceptor(type, def.nullable)), | ||
type, | ||
def.nullable |
There was a problem hiding this comment.
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.
The goal of this change is to save user defined value for further usage at runtime. If value is not directly provided - try to get it from TS metadata.
|
@ekzobrain Thank you for the explanation. I agree with your first point. As a side note (and not really relevant for this PR), for the dynamic composition, the typing information is just one thing in the sea of configurations. For that, a secondary set of configuration would be more appropriate IMO. |
I think the bindable observer itself doesn't need this much information to operate. I'd prefer we don't burden the observer with things it doesn't need to know. If the concerns are around retrieving the original configuration in authored code, we can assign the whole definition to the observer itself separately for tooling integration, such as storybook/devtool. // from controller
observer[Symbol.for('au:bindable')] = some_bindable_definition; |
Please ignore my previous comment, I was under the impression that the definition of a bindable was being retrieved from the observer. I'm all good with this direction, and nice refactoring around the get interceptor too. If you can add the tests, I'll do a review and we can get it in. There'll be a little conflict when you merge master, you can simply copy paste this code to resolve it export class BindableDefinition {
private constructor(
public readonly attribute: string,
public readonly callback: string,
public readonly mode: BindingMode,
public readonly primary: boolean,
public readonly property: string,
public readonly set: InterceptorFunc,
public readonly type?: PropertyType,
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);
return new BindableDefinition(
def.attribute ?? kebabCase(prop),
def.callback ?? `${prop}Changed`,
def.mode ?? BindingMode.toView,
def.primary ?? false,
def.property ?? prop,
def.set ?? getInterceptor(type, def.nullable),
type,
def.nullable
);
}
} @Sayan751 for the |
Don't close this one as stale, please. I will finish it up soon. |
It doesnt feel like you need this in the core @ekzobrain ? Shouldnt you be able to get the info from the definition on the controller? |
I think this might be a conflicting change wrt. to the migration to the TC39 decorator. The TS does not emit any longer design time metadata for non-legacy decorators. Hence, the dependence on the emitted metadata needs to go away, till TS decides again to emit the metadata. Refer: |
Pull Request
📖 Description
This change add persistence of bindable() type/nullable properties for later usage at runtime. Currently these properties are configurable, but not persisted, they are used just for coercer setup. Also Typescript decodator metadata types (if available) are saved as defaults. This is usefull for docs generators/etc. I currently need it for Aurelia Storybook plugin. May be later bindable validation according to these types may added as an opt-in functionality.
🎫 Issues
👩💻 Reviewer Notes
📑 Test Plan
⏭ Next Steps