[WIP] Flesh out Component/Route/Controller typings #24

theroncross · 2017-08-28T23:07:00Z

Add Component class properties, methods, and events
Refactor CoreView to be a private class with correct extends
Move private Component 'uses' outside exported namespace
Add Component tests and comments from Ember API docs
Uses the same pattern to get Controller and Route exporting local and inherited public properties and methods

General approach here was to use the extends and uses sections from the 2.14 api docs for structure:

became
class Component extends CoreView.extend(ViewMixin, ActionSupport, ClassNamesSupport) {...}
Note - api labeled private has not been included, but that includes TargetActionSupport#triggerAction which doesn't really seem private so much as 'not recommended'. 🤷‍♂️
I have NOT checked them against 2.15.
I added private classes and mixins outside the Ember namespace as interfaces with matching values (e.g. const ActionHandler: Ember.Mixin<ActionHandler>) where needed.
Unfortunately, the scope of this PR grew a bit bigger than I'd hoped, including removing a couple of unused namespaces. Happy to move those changes to another PR if it's a problem.

dfreeman · 2017-08-28T23:26:06Z

types/ember/index.d.ts

@@ -1538,6 +1684,7 @@ namespace Ember {
        */
        has(name: string): boolean;
    }
+    const Route: Mixin<Route>;


Route should still be a class, shouldn't it?

Sorry - didn't mean to include that, though I think I'll need to make some changes.

dwickern · 2017-08-28T23:31:02Z

types/ember/test/component.ts

+const component1 = Component.extend({
+  actions: {
+    hello(name: string) {
+      console.log('Hello', name);


It would be nice to test that the context gets set properly, e.g. by calling this.get('someComponentProperty')

dwickern · 2017-08-28T23:32:24Z

types/ember/index.d.ts

+    **/
+    actions: ActionsHash;
+}
+const ActionHandler: ActionHandler;


I think this should be Mixin<ActionHandler>

dwickern · 2017-08-28T23:33:05Z

types/ember/index.d.ts

+}
+const ActionSupport: ActionSupport;
+
+interface ClassNamesSupport {


Where does ClassNamesSupport and ActionSupport come from?

https://emberjs.com/api/ember/2.14/classes/Ember.ClassNamesSupport
https://emberjs.com/api/ember/2.14/classes/Ember.ActionSupport

Both are in the 'uses' list of https://emberjs.com/api/ember/2.14/classes/Ember.Component

Oh, ok, I was testing on ember 2.9, those must have been introduced later.

theroncross · 2017-08-29T01:28:10Z

The route tests won't really match the docs until we have #9 and #20 merged. At this point I'm getting some nice help from VS Code when extending Component with an ES6 class, but Route and Controller are missing something that I'm not seeing.

As soon as #25 is merged I'll rebase this branch.

theroncross · 2017-08-30T00:57:34Z

I need to grab the TemplateFactory type off of the htmlbars-inline-precompile module for the layout type in the Component class.

There's a PR in for grabbing types from function return values, but I'm not otherwise seeing a good way to do it without declaring a dummy type or just re-declaring the interface in the Ember module. Thoughts?

dwickern · 2017-08-30T01:23:00Z

You should be able to

import { TemplateFactory } from 'htmlbars-inline-precompile';

theroncross · 2017-09-05T23:09:11Z

I'm getting failing tests from ember-inflector and qunit. Happy to fix them up, but they're way outside of my already over-sized scope here. Suggestions?

dwickern · 2017-09-10T00:30:37Z

types/ember/index.d.ts

+}
+const ControllerMixin: Ember.Mixin<ControllerMixin>;
+
+namespace Ember {


@theroncross The tests pass locally for me if you change this line back to export namespace Ember and revert the ember-inflector tests changes in 5356bd0

Got it. I'm still getting Cannot find name..s in the ember-qunit declarations for Hooks, Assert, and Qunit. I've diffed everything I can think of to see if I've accidentally changed something. Where are those declared?

What code are you referring to?

theroncross · 2017-09-12T15:02:40Z

@dwickern I see that you added a static property to the Component class. Could you explain why?

dwickern · 2017-09-12T15:25:36Z

It's public API: Component.positionalParams

- [x] Add Component class properties, methods, and events - [x] Refactor CoreView to be a private class with correct extends - [x] Move private Component 'uses' outside exported namespace - [x] (Unfortunately) had to move Controller pieces, too. - [x] Add Component tests and comments from Ember API docs

The Controller and Route classes don't seem to be exporting all their members in the way the Controller is, but I'm lacking the bandwidth to spot the problem.

theseyi · 2017-09-30T08:57:26Z

types/ember/index.d.ts

-        sendAction(action: string, context: any): void;
-        targetObject: Controller;
-        static positionalParams: string | string[];
+    class Component extends CoreView.extend(ViewMixin, ActionSupport, ClassNamesSupport) {


Should there be support for dynamic/runtime keys on the component? There will obviously be a loss of type-safety since they will be typed as {[key: string]: any} or is the mandate that key's be static with a default value? I feel like the former is more in line with how Ember works {{my-component dynamicPropKey=anyValue}}, but I think I might prefer the latter because of type-checking...

Yeah, if you add the looser type you lose all help from the compiler for get and so on, alas.

For TS to be useful people will need to declare what the component expects as its API in the class definition for it, because while you can set arbitrary keys on a component invocation, they’re essentially meaningless: nothing will ever be done with them.

For cases where people want to support adding arbitrary values at a given key, they can type that key as variousData: any in the component declaration and then it’ll be available within the component methods.

theroncross · 2017-09-30T14:40:16Z

Transition isn't exported through any of the namespaced modules. Is that a problem?

dwickern · 2017-09-30T16:09:45Z

You're exporting it as Ember.Transition

LGTM

theroncross · 2017-09-30T16:13:42Z

I was thinking specifically of the case in the component tests where I have to import the whole module to access Transition. Aren't these eventually going to be pulled out into smaller files?

chriskrycho · 2017-09-30T16:18:03Z

SHIP IIIIIT!

dwickern · 2017-09-30T16:49:36Z

I suppose we could have a named export too. I don't want to do anything crazy, Transition is just a typescript interface, not a real API

dwickern · 2017-09-30T16:50:06Z

Thanks for solving the merge conflicts @theroncross !

dfreeman reviewed Aug 28, 2017

View reviewed changes

dwickern reviewed Aug 28, 2017

View reviewed changes

theroncross changed the title ~~Flesh out Component typings~~ [WIP] Flesh out Component/Route/Controller typings Aug 29, 2017

theroncross force-pushed the component-typings branch from 4084edf to 975f973 Compare September 5, 2017 22:29

theroncross changed the title ~~[WIP] Flesh out Component/Route/Controller typings~~ Flesh out Component/Route/Controller typings Sep 5, 2017

theroncross changed the title ~~Flesh out Component/Route/Controller typings~~ [WIP] Flesh out Component/Route/Controller typings Sep 5, 2017

dwickern reviewed Sep 10, 2017

View reviewed changes

theroncross force-pushed the component-typings branch from 5356bd0 to b23ef2c Compare September 15, 2017 13:18

dwickern mentioned this pull request Sep 19, 2017

Issues with current Route definitions #48

Closed

theroncross added 13 commits September 29, 2017 16:12

Fix bad Ember.Inflector references in ember-inflector tests

2a20f2a

Refactor ActionHandler as a mixin

77d341b

Refactor Route as a class

39fa395

Refactor component tests as classes

e8e19a7

Fix linting errors

f476982

Add Controller and Route tests

c60e8db

The Controller and Route classes don't seem to be exporting all their members in the way the Controller is, but I'm lacking the bandwidth to spot the problem.

Fix component.layout

010317a

Update route methods and properties

5d8c3a6

Remove GlobalArray reference

4ab4f1d

Use Component.extends now that it works

5e9e89f

Update Ember.Controller methods and props

ff769b3

Fix non-simple array type

edce0a2

theroncross added 6 commits September 29, 2017 16:21

Fix failing array test

f7e0429

Revert to exporting ember namespace

3be5413

Minor test fixes

0e78cea

Re-add static to component.positionalParams

8570fad

Set Component.layout type to TemplateFactory

644872b

Fix inconsistent indentation

d2c4706

theseyi reviewed Sep 30, 2017

View reviewed changes

Rebase with minor route fix

a9677ce

theroncross force-pushed the component-typings branch from b23ef2c to a9677ce Compare September 30, 2017 14:09

dwickern merged commit 11344dc into typed-ember:master Sep 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Flesh out Component/Route/Controller typings #24

[WIP] Flesh out Component/Route/Controller typings #24

theroncross commented Aug 28, 2017 •

edited

Loading

dfreeman Aug 28, 2017

theroncross Aug 28, 2017

dwickern Aug 28, 2017

dwickern Aug 28, 2017

dwickern Aug 28, 2017

theroncross Aug 29, 2017

dwickern Aug 29, 2017

theroncross commented Aug 29, 2017 •

edited

Loading

theroncross commented Aug 30, 2017 •

edited

Loading

dwickern commented Aug 30, 2017

theroncross commented Sep 5, 2017 •

edited

Loading

dwickern Sep 10, 2017

theroncross Sep 15, 2017

dwickern Sep 15, 2017

theroncross commented Sep 12, 2017

dwickern commented Sep 12, 2017

theseyi Sep 30, 2017

chriskrycho Sep 30, 2017 •

edited

Loading

theroncross commented Sep 30, 2017

dwickern commented Sep 30, 2017

theroncross commented Sep 30, 2017

chriskrycho commented Sep 30, 2017

dwickern commented Sep 30, 2017

dwickern commented Sep 30, 2017

[WIP] Flesh out Component/Route/Controller typings #24

[WIP] Flesh out Component/Route/Controller typings #24

Conversation

theroncross commented Aug 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theroncross commented Aug 29, 2017 • edited Loading

theroncross commented Aug 30, 2017 • edited Loading

dwickern commented Aug 30, 2017

theroncross commented Sep 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theroncross commented Sep 12, 2017

dwickern commented Sep 12, 2017

Choose a reason for hiding this comment

chriskrycho Sep 30, 2017 • edited Loading

Choose a reason for hiding this comment

theroncross commented Sep 30, 2017

dwickern commented Sep 30, 2017

theroncross commented Sep 30, 2017

chriskrycho commented Sep 30, 2017

dwickern commented Sep 30, 2017

dwickern commented Sep 30, 2017

theroncross commented Aug 28, 2017 •

edited

Loading

theroncross commented Aug 29, 2017 •

edited

Loading

theroncross commented Aug 30, 2017 •

edited

Loading

theroncross commented Sep 5, 2017 •

edited

Loading

chriskrycho Sep 30, 2017 •

edited

Loading