Skip to content
This repository has been archived by the owner on Sep 9, 2022. It is now read-only.

WIP: TypeScript #5

Closed
wants to merge 3 commits into from
Closed

WIP: TypeScript #5

wants to merge 3 commits into from

Conversation

dmzza
Copy link
Contributor

@dmzza dmzza commented Apr 5, 2019

index.d.ts is the TypeScript definition file, and that works for me in my project.

But I also want to write tests for Native Classes in TypeScript, and I'm stuck trying to make that work. Thankfully, @chriskrycho volunteered to help me out with this next week.

It also might be nice to add TypeScript examples to the README, but I haven't started that yet.

Once finished, this will close #4

TODO

  • TypeScript definition file
  • Tests in TypeScript
  • dtslint
  • README examples

unfortunately, `ember test` won't run at all, and I have a lot of errors.
This will have to wait until I get help.
@josemarluedke
Copy link

This would be very welcomed!

For contrast, here is my type definition file for this project:

declare module 'ember-oo-modifiers' {
  export default class Modifier<T = object, T2 = unknown[]> {
    public element: HTMLElement | SVGElement;

    public constructor(attrs: T, _owner: unknown);
    public static modifier(Klass: unknown): unknown;

    public didInsertElement(positional: T2, args: T): void;
    public didRecieveArguments(positional: T2, args: T): void;
    public didUpdateArguments(positional: T2, args: T): void;
    public willDestroyElement(positional: T2, args: T): void;
  }
}

usage:

import Modifier from 'ember-oo-modifiers';

interface Args {
  duration?: number;
  delay?: number;
}

class FadeUpModifier<T extends Args, T2 extends unknown[]> extends Modifier {
  public didInsertElement(positional: T2, options: T): void {
    //....
  }
}
// ....

@sukima sukima added the help wanted Extra attention is needed label Apr 20, 2019
@sukima
Copy link
Owner

sukima commented Apr 20, 2019

For anyone curious, I have a total lack of TS knowledge to be able to move this along. I need help from others smarter then me.

If you have TS knowledge and know how to help move this along please offer some guidance. I will not hesitate to merge this (provided it is ready).

@chriskrycho
Copy link

Sorry about the delay here; I will block out two hours this coming week to help land this robustly!

willDestroyElement(positionalArgs?: Array<any>, namedArgs?: { [key: string]: any; }): void;

static modifier(klass: typeof Modifier): typeof Modifier;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, there have been some changes on master. The typing is now:

export interface Args {
  positional: any[];
  named: { [key: string]: any };
}

export default class Modifier {
  args: Args;
  element: Element | null;
  isDestroying: boolean;
  isDestroyed: boolean;
  constructor(owner: Owner, args: Args);
  didRecieveArguments(): void;
  didUpdateArguments(): void;
  didInstall(): void;
  willRemove(): void;
  willDestroy(): void;
}

There is also the classic API too, but I have no idea what's the best practice for typing things that inherit from Ember.Object. Maybe @chriskrycho will know.

Choose a reason for hiding this comment

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

I’ll take a look and make a recommendation that way tomorrow!

Copy link

@chriskrycho chriskrycho Sep 20, 2019

Choose a reason for hiding this comment

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

The types should be a single index.d.ts file with the following definitions:

interface ModifierArgs {
  positional: unknown[];
  named: { [key: string]: unknown };
}

interface IModifier<Args extends ModifierArgs = ModifierArgs> {
  args: Args;
  element: Element | null;
  isDestroying: boolean;
  isDestroyed: boolean;
  didReceiveArguments(): void;
  didUpdateArguments(): void;
  didInstall(): void;
  willRemove(): void;
  willDestroy(): void;
}

type Owner = unknown;

declare module "ember-class-based-modifier" {
  export default class Modifier<Args extends ModifierArgs = ModifierArgs>
    implements IModifier<Args> {
    args: Args;
    element: Element | null;
    isDestroying: boolean;
    isDestroyed: boolean;
    constructor(owner: Owner, args: Args);
    didReceiveArguments(): void;
    didUpdateArguments(): void;
    didInstall(): void;
    willRemove(): void;
    willDestroy(): void;
  }
}

declare module "ember-class-based-modifier/classic" {
  import EmberObject from "@ember/object";

  export default class Modifier extends EmberObject implements IModifier {
    args: ModifierArgs;
    element: Element | null;
    isDestroying: boolean;
    isDestroyed: boolean;
    didReceiveArguments(): void;
    didUpdateArguments(): void;
    didInstall(): void;
    willRemove(): void;
    willDestroy(): void;
  }
}

There are a number of important things to note here.

First, I've replaced any with unknown. This means that TS consumers will need to do a runtime check to narrow the types from "anything" to "the actual type". (There are some features landing in TS 3.7 which will make Ember's debug-only assert work perfectly for this, so keep your eyes open for that!) This is roughly what you have to do in a well-behaved modifier anyway, so it's not an extra burden, just helpful guidance from the type system.

Second, this sets up the Args definition with a generic. This is so that the user can extend it with their own args in a way analogous to how the types for @glimmer/component work. Users of the modern, class-based API can write something like this, and it'll just work (with the caveat that they'll have to do exactly those runtime checks required by using unknown above):

import Modifier from 'ember-class-based-modifier';

export default class MyModifier extends Modifier {
  // ...
}

However, they'll also be able to do this, supplying a narrower type:

import Modifier from 'ember-class-based-modifier';

interface MyArgs {
  positional: [number, boolean],
  named: {
    first: string;
    second: object;
  }
}

export default class MyModifier extends Modifier<MyArgs> {
  // ...
}

Importantly, that will not type-check if the interface passed into the generic is not compatible with the required ModifierArgs type. (Note here that TS users will likely want to use debug assert on those arg types in the constructor until we have a solution in place for type-checking template invocation, which is unlikely to happen this calendar year.)

Third, that second point is only applicable to native classes. We don't have a way to make TypeScript's type system support that kind of use of generics with classic classes in a straightforward way, and we're not going to invest the time to try to work around it since the community is actively moving toward native classes—and classic classes have basically been non-starters for TS consumers for a long time anyway, for precisely this reason. Having the API in place as we do here is good, and this level of support matches the second-class

Fourth, and this is the most important, we don't have a way to test this at present. As such, please request a review from me or one of the other Typed Ember contributors on changes to this file. I'll be putting together guidance over the next few months about how to use dtslint or tsd (or the two in combination) to provide tests for the exported types, which should help with this!

Choose a reason for hiding this comment

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

One addendum: that type Owner = unknown can be replaced with an import if we have a public API import for Owner; I just don't know where that exists if so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @chriskrycho

I'll update this branch with your recommendations.

I will also add your examples to the README.

@dmzza dmzza mentioned this pull request Sep 20, 2019
2 tasks
@dmzza
Copy link
Contributor Author

dmzza commented Sep 20, 2019

I essentially started over on this and made a new PR #20. My original branch remains intact just in case anyone was depending on it.

@dmzza dmzza closed this Sep 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeScript Definition
5 participants