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

[ekzobrain]

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

[ekzobrain]

@Sayan751 @bigopon

[bigopon]

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.

[ekzobrain]

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

[Sayan751]

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.

[Sayan751]

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?).

[ekzobrain]

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.

[ekzobrain]

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

[Sayan751]

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.

[ekzobrain]

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.

[Sayan751]

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

[bigopon]

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.

[Sayan751]

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

[Sayan751]

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

[ekzobrain]

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.

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.
This way developers are able to access type information at runtime. This might be usefull for several purposes:

1. Docs generation. Currently the most widespread approach for doc generation is source code analisys which emits some configuration files, which are then displayed by UI. Storybook works the same for most frameworks - it uses document generation, and then parses it's config files to display docs and create interactive controls for component live examples. But that brings too much complexity to framework plugins and also some limitations regarding hot reload (one needs to rebuild documentation every time he wants changes to display in SB UI). I created an Aurelia 2 Storybook plugin, which avoids using docs generation step, it gets all component information at runtime, so it is very lightweight, but no no less powerfull. It gets attribute list from BindableDefinition, default values and comments by parsing component AST, and bindable types from TS metadata with Metadata.get. But it is also nessesary to know if developer has actually overriden TS type configuration with type bindable config. Also in plain JS this is the only way to let type information be available at runtime.
   You may see plugin usage examples here: https://ekzo-dev.github.io/aurelia-components/?path=/story/introduction--page
   And source code here: https://github.com/ekzo-dev/aurelia-components

2. Type information is helpfull for dynamic composition, like dynamic form generators, and vice versa. This may involve runtime type validation, which can be now implemented using this stored metadata.

[Sayan751]

@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.

[bigopon]

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;

[bigopon]

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 null vs undefined, I think we can opt for simplicity every where it doesn't matter, that kind of means anything except bindings and observers. A lot less to read between != null vs !== void 0 && !== null

[ekzobrain]

Don't close this one as stale, please. I will finish it up soon.

[bigopon]

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?

[Sayan751]

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:

• Reflect Metadata not supported for TC39 decorators microsoft/TypeScript#55788
• Expose design-time type information in TC39 decorator metadata when emitDecoratorMetadata: true microsoft/TypeScript#57533