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.

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

Type safe Input setting on components created with createComponent #51878

Open
cusher opened this issue Sep 22, 2023 · 9 comments
Open

Type safe Input setting on components created with createComponent #51878

cusher opened this issue Sep 22, 2023 · 9 comments
Labels
Milestone

Comments

@cusher
Copy link

cusher commented Sep 22, 2023

Which @angular/* package(s) are relevant/related to the feature request?

core

Description

When a component is created via the createComponent method on ViewContainerRef, there are two ways inputs can currently be set:

  1. Setting properties directly on the component instance. This seems to generally not be recommended due to the changes not respecting lifecycle hooks.
  2. Using the setInput method on the ComponentRef. This seems to be the recommended method, however it comes with its own set of issues:
    a. The name can be any string, and thus is not guaranteed to be the name of an actual input.
    b. The value type is unknown, so even if you are always using the right names, there is no way to have any sort of type-checking on the value itself.

Additionally, for both options, because the inputs are being set one-by-one, there is no way to check for missing required inputs.

As a result, setting inputs on components created with createComponent is quite a bit riskier than using components in templates. It would be great if the API for creating component instances dynamically in TypeScript felt just as robust as instantiating a component inside of a template. Additionally, improvements here this could trickle down to provide a better interface for setting inputs in other use cases e.g. Dialog creation in Angular Components.

Proposed solution

Add an inputs field to the options parameter of createComponent which allows passing in initial values for each input on the component. Ideally, this should:

  1. Have full type info matching the inputs on the component itself.
  2. Respect the required flag, so if an required input is missing, an error is passed along.

Alternatives considered

  • Alternative signature for setInput on ComponentRef which allows setting inputs in a type safe manner. This would be a more flexible option than the proposed solution, but has no ability to check if required inputs have been set.
@JeanMeche
Copy link
Member

JeanMeche commented Sep 22, 2023

This is hardly achievable since the input can be aliased an this is not represented in the typings. This is the same thing for required inputs, it's not represented in the type that the decorator has required: true (remember decorators have no effect on typings).

@cusher
Copy link
Author

cusher commented Sep 23, 2023

Ah. Is the AOT compiler is only able to catch and report problems in templates, and not in the TS code? I was hoping that since these sorts of errors bubble up in templates, that it would also be feasible to bubble them up in TS. If that's not the case, then I see how that's a problem...

That being said, the current method of setInput is also not very dependable at scale, IMO. It is very reliant on testing to avoid bugs that "should" be caught by type checking, and even if you you do have extensive tests it breaks down if you make a change without properly updating tests, or do update tests but fail to update every invocation outside of tests. (When recently updating a project code-base to move from string keys to properties w/ type inference in another aspect, there were a shocking number of bugs that had crept in over the years for this exact reason that we discovered all at once.) So I'm reluctant to ever use something like that without strong guardrails in place.

If it's just not feasible to add something more rigid, then it is what it is, we'll probably just continue using the props of the component instance with workarounds for our own use cases. Thanks for the response.

@BojanKogoj
Copy link

I'm currently facing the same issue. setInput() offers almost no guarantees, other than runtime exception for incorrect property: https://stackblitz.com/edit/stackblitz-starters-6zmbjw?file=src%2Fmain.ts

If nothing else, some way to make input required would be nice.

@dylhunn dylhunn added area: core Issues related to the framework runtime core: dynamic view creation labels Sep 27, 2023
@ngbot ngbot bot modified the milestone: needsTriage Sep 27, 2023
@SebastianPodgajny
Copy link

SebastianPodgajny commented Oct 13, 2023

@BojanKogoj @cusher here is my take on type safe component inputs https://stackblitz.com/edit/stackblitz-starters-vlhdvr?file=src%2Fmain.ts

@cusher
Copy link
Author

cusher commented Oct 13, 2023

@SebastianPodgajny Really cool, impressive to see a purely TS solution!

It would be great if there was a way to get this to work with existing components using regular @Inputs without needing to declare a separate NgInputs field... but it's not clear to me how that could be accomplished (or if it's even feasible).

Maybe there's a way to leverage TS decorators for that? It seems at least somewhat plausible to me that the @Component decorator could "generate" extra type information for each of the @Inputs, then a similar scheme to what you provided could be used from there on... but easier said than done.

@Harpush
Copy link

Harpush commented Feb 29, 2024

Can it be a specific signal inputs feature? With the new input function both typings and required can be inferred I believe. This issue seems really important and the new changes complement it.

@Harpush
Copy link

Harpush commented Mar 1, 2024

Proposal:

I did some type gymnastics to try and type signal inputs correctly for this issue to be feasible. The two main ideas are type safety in setInput and an option to set inputs (especially required) at createComponent call site. For brevity (it is possible though) I omitted model and just handled input. The idea is looking just on signal based input/models and not on decorators.

The end result should be:

const cp = this.vcr.createComponent(Counter, {
  signalInputs: {
    a: 1,
    c: 3
  }
});

cp.setSignalInput('a', 2);
cp.setSignalInput('c', 2);

// Hopefully possible too
cp.setSignalInputs({
  a: 2,
  c: 2,
});

The important parts are:

  1. setSignalInputs full type safety - meaning a is the property name or alias if exists
  2. signalInputs is required and will be either an empty object with optional properties if no required inputs or an object with required inputs as required props. Here too aliases will work correctly in a type safe way.

Changes in Angular

The interface InputSignalWithTransform is the base interface which will change to inherit from a "real" base interface:

interface BaseInputSignal<
  ReadT,
  WriteT,
  RequiredT extends boolean,
  AliasT extends string
> {
  [SIGNAL]: InputSignalNode<ReadT, WriteT>;
  [ɵINPUT_SIGNAL_BRAND_READ_TYPE]: ReadT;
  [ɵINPUT_SIGNAL_BRAND_WRITE_TYPE]: WriteT;
  [ɵINPUT_SIGNAL_REQUIRED]: RequiredT;
  [ɵINPUT_SIGNAL_ALIAS]: AliasT;
}

This allows InputFunction to be a fully type safe interface in terms of required and alias.
The interface InputOptions will need to know the new alias type too:

interface InputOptions<ReadT, WriteT, AliasT extends string> {
  alias?: AliasT;
  transform?: (v: WriteT) => ReadT;
}

The usage will be almost similar to now but will return a more specific type:

// BaseInputSignal<number | undefined, number | undefined, false, never>
input<number>();
// BaseInputSignal<number, number, false, never>
input(2);
// BaseInputSignal<string, number, false, never>
input('2', { transform: (n: number) => n.toString() });
// BaseInputSignal<string, number, false, 'hello'>
input('2', {
  transform: (n: number) => n.toString(),
  alias: 'hello',
});
// BaseInputSignal<number, number, false, 'hello'>
input(2, { alias: 'hello' });
// BaseInputSignal<number, number, true, never>
input.required<number>();
// BaseInputSignal<string, number, true, never>
input.required({
  transform: (n: number) => n.toString(),
});
// BaseInputSignal<string, number, true, 'hello'>
input.required({
  transform: (n: number) => n.toString(),
  alias: 'hello',
});

The only missing declaration is required input with alias. Currently it is declared as input.required<number>({alias: 'hello'}) but this is not possible with typed alias and will require: input.required<number, 'hello'>({alias: 'hello'}) which isn't a good idea.
For this case I propose a new option just for this case called type which will use a simple function const type = <T>(): T => undefined as T; and will be used like this:

// BaseInputSignal<number, number, true, 'hello'>
input.required({ type: type<number>(), alias: 'hello' });

The actual InputFunction will look like this:

interface InputFunction {
  <ReadT>(): BaseInputSignal<
    ReadT | undefined,
    ReadT | undefined,
    false,
    never
  >;
  <ReadT, TAlias extends string = never>(
    initialValue: ReadT,
    opts?: InputOptionsWithoutTransform<ReadT, TAlias>
  ): BaseInputSignal<ReadT, ReadT, false, TAlias>;
  <ReadT, WriteT, TAlias extends string = never>(
    initialValue: ReadT,
    opts: InputOptionsWithTransform<ReadT, WriteT, TAlias>
  ): BaseInputSignal<ReadT, WriteT, false, TAlias>;
  required: {
    <ReadT, TAlias extends string = never>(
      opts?: InputOptionsWithoutTransform<ReadT, TAlias> & {
        type?: ReadT;
      }
    ): BaseInputSignal<ReadT, ReadT, true, TAlias>;
    <ReadT, WriteT, TAlias extends string = never>(
      opts: InputOptionsWithTransform<ReadT, WriteT, TAlias>
    ): BaseInputSignal<ReadT, WriteT, true, TAlias>;
  };
}

At this point all the parts are ready and we can type the new setSignalInput and signalInputs options.

// Helpers for the BaseInputSignal
type GeneralBaseInputSignal = BaseInputSignal<any, any, any, any>;
type BaseInputSignalAlias<T> = T extends BaseInputSignal<any, any, any, infer T>
  ? T
  : never;
type BaseInputSignalRequired<T> = T extends BaseInputSignal<
  any,
  any,
  infer T,
  any
>
  ? T
  : never;
type BaseInputSignalWrite<T> = T extends BaseInputSignal<any, infer T, any, any>
  ? T
  : never;

// Properties info extraction (mainly alias and required related)
type AllInputSignalPropNames<CompT> = {
  [P in keyof CompT]: CompT[P] extends GeneralBaseInputSignal ? P : never;
}[keyof CompT];
type AllInputSignalAliasToPropNameMap<CompT> = {
  [P in AllInputSignalPropNames<CompT> as BaseInputSignalAlias<
    CompT[P]
  > extends never
    ? P
    : BaseInputSignalAlias<CompT[P]>]: P;
};
type InputSignalAliasByRequired<CompT, RequiredT extends boolean> = {
  [AliasT in keyof AllInputSignalAliasToPropNameMap<CompT>]: BaseInputSignalRequired<
    CompT[AllInputSignalAliasToPropNameMap<CompT>[AliasT] & keyof CompT]
  > extends RequiredT
    ? AliasT
    : never;
}[keyof AllInputSignalAliasToPropNameMap<CompT>];

// Won't accept comp as will be implicit due to being in ComponentRef
const setSignalInput = <
  CompT,
  AliasT extends keyof AllInputSignalAliasToPropNameMap<CompT>
>(
  comp: Type<CompT>,
  key: AliasT,
  value: BaseInputSignalWrite<
    CompT[AllInputSignalAliasToPropNameMap<CompT>[AliasT] & keyof CompT]
  >
) => undefined;

// Will be the type of the new property inside createComponent
type SignalInputs<CompT> = {
  [P in InputSignalAliasByRequired<CompT, true>]: BaseInputSignalWrite<
    CompT[AllInputSignalAliasToPropNameMap<CompT>[P] & keyof CompT]
  >;
} & {
  [P in InputSignalAliasByRequired<CompT, false>]?: BaseInputSignalWrite<
    CompT[AllInputSignalAliasToPropNameMap<CompT>[P] & keyof CompT]
  >;
};

With this we have fully type safe dynamic components with signal inputs 😀
@dylhunn I hope this can help the idea being implemented

@MaximSagan
Copy link

@JeanMeche @pkozlowski-opensource Any thought on the above proposal or something similar? Input metadata integrated into the typing seems the way to go here.

@MaximSagan
Copy link

I'm currently facing the same issue. setInput() offers almost no guarantees, other than runtime exception for incorrect property

It's not even a runtime exception. Just a console error, so we can't even capture it. Would be nice if setInput() at least returned a boolean indicating whether the operation was successful or not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants