Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

AngularJs TypeScript typings and --strictFunctionTypes #16617

Open
1 of 3 tasks
rkirov opened this issue Jun 28, 2018 · 3 comments
Open
1 of 3 tasks

AngularJs TypeScript typings and --strictFunctionTypes #16617

rkirov opened this issue Jun 28, 2018 · 3 comments
Milestone
1.8.x - won't fix

Comments

@rkirov
Copy link
Contributor

rkirov commented Jun 28, 2018 •
edited

I'm submitting a ...

  • bug report
  • feature request
  • design discussion

TypeScript introduced a new strictness flag with TS 2.7 - https://github.com/Microsoft/TypeScript/wiki/What%27s-new-in-TypeScript#strict-function-types

It makes typechecking for callback based APIs much stricter. For example:

function f(mixedArray: (string|number)[]) {
  mixedArray.map((x: string) => x.charAt(0));
}

only errors when --strictFunctionTypes is turned on.

AngularJs has many callback based APIs, which will be checked at more strictly with the flag. Some real bugs will be exposed, but also it will start erring on many correct patterns. For example:

{
  require: 'ngModel'
  link: (...., controller: NgModelController) {...}

This is now an error because TS doesn't know that require and link are related. As written the typings require one handle the full spectrum of options in the link function IController | IController[] | {[key: string]: IController}.

TS types are quite expressive and @gkalpak and I tried to write up some prototype of typings that are aware of the link between require and link:

interface IDirective<T extends string|string[]> {
  link?: (x: LinkT<T>) => void;
  require?: T;
}

type LinkT<T> =
    T extends string ? IController : T extends string[] ? IController[] : never;

But even with full conditional types, it is impossible to extend this for user-defined directives.

So as plan B, I am going to contribute some nice type aliases to DefinitelyTyped type LinkFnControllerType = IController | IController[] | {[key: string]: IController}
and propose the canonical solution to be writing the link function as:

link: (..., controller: angular.LinkFnControllerType) => {
  let myController = controller as MyControllerType;
}

One added benefit is that there is an explicit 'as' cast to remind the reader to check with the 'require' and controller statements.
rkirov added a commit to rkirov/DefinitelyTyped that referenced this issue Jun 28, 2018
@rkirov
Introduce two new type aliases - LinkFnControllerType and RequireType.
67132da 
This allows users to write callbacks using those aliases instead of the
full type.

Having convinient aliases is required for having AngularJS in TS work
better with strictFunctionTypes flag on.
see: angular/angular.js#16617

Also having them next to each other shows the symmetry between the
require and controller param better.
rkirov added a commit to rkirov/DefinitelyTyped that referenced this issue Jun 28, 2018
@rkirov
(angular.js) Introduce type aliases LinkFnControllerType and RequireT…
dcfb038 
…ype.

This allows users to write callbacks using those aliases instead of the
full type.

Having convinient aliases is required for having AngularJS in TS work
better with strictFunctionTypes flag on.
see: angular/angular.js#16617

Also having them next to each other shows the symmetry between the
require and controller param better.
@rkirov rkirov mentioned this issue Jun 28, 2018
Closed
9 tasks
@gkalpak
Copy link
Member

gkalpak commented Jun 28, 2018

If one is fine with having to pass one extra type argument to IDirective (i.e. the share of require), then we could avoid the "casting" by specifying a list of "known controllers" (for built-in directives) and allow the user to expand the list with their own directives.

The main benefits are that they don't have to cast at all when requiring built-in directives (and still get the correct types) and only having to define the mapping (directive name --> controller type) once for their custom directive.

By being more specific about the shape of your require property, you will also get better type-checking for your controllers argument (e.g. not only {[key: string]: IController} but {ngModel: INgModelController} which also checks the keys).

This is what I am talking about: poc

I could find a way to auto-infer the controller types for arrays, but we might be able to cover the most common cases (e.g. arrays of length 2, 3, 4). (For larger arrays, you would get the broader IController type.)

The main downside (that I can think of) is having to provide the extra type argument to IDirective.

@thorn0
Copy link
Contributor

thorn0 commented Jun 28, 2018

@gkalpak Keep in mind that the keys of the require object aren't (don't have to be) the same as the names of the required directives:

require: { myProp: 'ngModel', yourProp: '^^someOtherDirective' }

So I don't think anything can be done here.

link: (..., controller) => {
  const myController = controller as MyControllerType;
}

This (without the redundant type annotation) is indeed the right thing to do.

@gkalpak
Copy link
Member

gkalpak commented Jun 28, 2018

Nooo 😢 You ruined everything 😭
And apparently TypeScript doesn't allow (which wouldn't be 100% accurate anyway):

{
  [key in keyof T]: T[key] extends keyof IKnownControllers ?
    IKnownControllers[T[key]] :    <-- Error: "T[key] can't be used to index IKnownControllers" :(
    IController
}

We might still be able to use the approach for string and string[] (or pre-defined common lengths), but again there is not way to correctly type the directive API 😁
So, probably not worth trying too hard 😛

@gkalpak gkalpak added this to the 1.7.x - won't fix milestone Sep 26, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.8.x - won't fix
Development

No branches or pull requests
3 participants
@thorn0 @rkirov @gkalpak