[types] Ensure we export symbols that are appropriately types, values or both

[mike-north]

trafficstars

Which package(s) does this problem pertain to?

• [x] @types/ember
• [x] @types/ember__string
• [x] @types/ember__polyfills
• [x] @types/ember__object
• [x] @types/ember__utils
• [x] @types/ember__array
• [x] @types/ember__engine
• [x] @types/ember__debug
• [x] @types/ember__runloop
• [x] @types/ember__error
• [x] @types/ember__controller
• [x] @types/ember__component
• [x] @types/ember__routing
• [x] @types/ember__application
• [x] @types/ember__test
• [x] @types/ember__service
• [x] @types/ember-data
• [ ] @types/rsvp
• [x] @types/ember-test-helpers
• [x] @types/ember-testing-helpers
• [ ] Other
• [ ] I don't know

What are instructions we can follow to reproduce the issue?

example repro case: I want to acquire a type that represents an instance of the ember-data store.

Today I have to do something like

type Store = InstanceType<typeof Store>;

This is because Store represented as only a value. Passing it through the InstanceOf<typeof x> effectively converts the value to a factory type, and then gets the instance produced by the factory.

Now about that bug. What did you expect to see?

All services, etc.. should behave just like native classes. No type query or instanceof should be required for use

CC: @runspired

[chriskrycho]

Iiiiinteresting, and good find. Have you started on implementing the fix for DT? I can help contribute to that, but not till the end of the month or beginning of August.

[dfreeman]

I'm not sure I follow the example you're giving with Store — that's a class declaration in @types/ember-data, so shouldn't it already be exporting identifiers in both the type and value namespaces?

I see the behavior I'd expect spinning up a fresh app with ember-cli-typescript, i.e. this typechecks:

import Store from 'ember-data/store';

function foo(store: Store) {
  store.findRecord('some-model', 123);
}

foo(new Store());


type MyStoreAlias = Store;

function bar(store: MyStoreAlias) {
  store.findRecord('some-model', 123);
}

bar(new Store());

Why do you need to 'strip away' the Store identifier in the value namespace?

[mike-north]

@dfreeman I'll have to dig a bit deeper, and I'm sure you're correct about the current behavior. That being said, anytime a InstanceType<typeof T> is necessary, it's a sure sign that a factory isn't declaration-merging with its instance interface properly. I think the root cause has to do with the return of Ember.Object.extend

@chriskrycho I have not started a fix -- this is mostly just to track ugly things that lib authors are having to do as a result of the way the types are currently written.

[chriskrycho]

So I think the actual (or at least one major contributor to the) problem here for Ember Data at least is that a bunch of the types are present on the namespace but not in the main ember-data module. If you import them from the now-legacy module re-export locations, you get the actual types, but if you try to pull them from the root, you don't have access to types, because namespaces only give you access to values, not types, within them.

I'm going to try tackling the full re-exports situation for the new module layout on Friday, as I have time available for it; I'll see if I can make this problem go away as well.

[chriskrycho]

Addendum to the above: I don't know how the re-exports are making that actually work if the above is true, though; that doesn't actually make sense to me. I'll figure it all out on Friday, though.

[mike-north]

namespaces only give you access to values, not types, within them.

This doesn't have anything to do with namespaces, it has to do with the various types of things that can co-exist on a TypeScript symbol.

If we have something like this defined or imported from elsewhere

class Car {
   static isVehicle = true
   color!: string;
   doors!: number;
}

and then use it as follows it will fail

declare namespace ConstReExports {
   export const car: Car;
}

// fails because const declarations are just _values_
let a: ConstReExports.car = new Car();
let b = ConstReExports.car; // treating as value is ok

similarly, if we re-export an interface, we'll run into problems on the value side

declare namespace InterfaceReExports {
   export interface car extends Car {}
}

// treating as a type is ok
let c: InterfaceReExports.car = new Car();
let d = InterfaceReExports.car; // treating as value fails

We need a value and a type to ride along on the same symbol. The two options are to use a class (which does this for us automatically)

declare namespace ClassReExports {
   export class car extends Car { }
}

// treating as a type is ok
let e: ClassReExports.car = { color: 'red', doors: 4};
let f = ClassReExports.car; // treating as value is ok

or use some more obvious declaration merging to create an identical result

declare namespace MergedReExports {
   export const car: Car;
   export interface car extends Car {}
}

// treating as a type is ok
let g: MergedReExports.car = { color: 'red', doors: 4};
let h = MergedReExports.car; // treating as value is ok

https://www.typescriptlang.org/play/index.html#code/MYGwhgzhAEDCYCdoG8CwAoaWIBcw4EthoCIA1AUwAsiQLoBeaHBAVwoy2mAHsQeEAQgBc0XAgIA7AOYBuTlgAmPARBHRJrALYAjCgnnoAvhgyKKoRPUlgtFCAAcwwerB6TcAJQoBRAB4OAjgwaJhYFAFB3O643Iii8AYYJugYAPRp0ABmYAQgMHrAYKwQ9LweONDmlgj4BDHQVtAAViWVAPoAbmAg7BDtGHSVYAkxON7+gQjBAHRFSEySFADucIgAFACUhkPQOoxwYxOR0xBziLLQGcwIFHUyjTDdvfSk0DwA1qapP9XgtxpbPYnC5oABJSQ4fQ5FzHKbBFAKaAReEkSHQ5xlRDIvxQySKGCJFApFLpTIsO6EB6QRrMACeDleME+gwolWAoghUIQMIocKCZ3mByWq0SWx2bKqBy5GNhvhOs3ml2uFPu0ke0Ge7GyuXy3zMFn+1iBjkxcHAUH5p0RYRxqMsUDiSAieIJayQyGgJO+Ktuao1YHpjJIzK+6F2FASFogVsV2KYnt4-AQogA5LdFKmADRVFQICCiAAsRgllSyB1g0djgouV3Jfqp6ppWqZ7zD+vQfyaNjsptBAFl9NIKIpqzauCiouVYvMEhckZPpmjubynTjXYTschvT9fZSpE2YIGcAzWyzw5LpKJBwhh6P5fCawsUNFk2mM9nc6oiyXWZUqAcN53tW5wGHWNz7tSTw9NqbznkAA

we have this problem all over the place in our types, and in related packages (rsvp, maybe qunit?) too. Here's a quick explanation of how we can connect this to the InstanceType<typeof> stuff mentioned in this issue

// THIS is how we often re-export something, and it'll cause problems
const car = Car; 

let car1: car; // fails. car is a value, not a type

// passes, type queries let us get from value -> type
let car2: typeof car; 


// fails. Oops! It turns out this is the type of the FACTORY
car2 = new Car();
car2.isVehicle; // passes. Yep, definitely the factory

// Ok now we use the InstanceType<typeof> shenanegans to get the instance's type
let carInstance: InstanceType<typeof Car>;
carInstance = new Car(); // passes

https://www.typescriptlang.org/play/index.html#code/MYGwhgzhAEDCYCdoG8CwAoaXrAPYlwQEIAuaCAFwQEsA7AcwG4NtoATXQiU6WgVwC2AIwCmCZuhbZKYCtWDRqEAGoiAFvJAjoAXmhU+IiQF8MGPLUo5EuuIkbQA9I+gAVABIBJAMqKYa3AB3aEDtXAAzChFaaAQRAFoRAA8AB0IKM3QtCmsEAEYyYHsnF3CwahAIADo7JCVoMGgANzAQQ0znaBTICBEIABp9AE8U7QBHQxo+6GzoPhh6ERzwhFwBZtbDaHiAPmHRjFmihAAmMgoRkQjch0yMTrKK6ugAeVwU7mhPHIo+BEtoLg+D8NDB6hQ1NoLqNAeF9JDoAAxACCsFcLwASgBNcyIE62WgiYLwBAACgAlBJjicqkpVBpQEYSl0en0aliRCkamwROE6NQoiAhvDtGVgBRCEMOi4XgBrXhBELaeZQhGeSwUMC0YBXcIAHmhur2EEhtC1InoWpgEugixB2joMm1IgA5NbLoclrl1U6dWQfZrna5LgbLtcSTsqYgA1qdQSibUKQ5Ot0oH1MkA

So, in summary, I think we need two distinct improvements for separate reasons

(1) We need to move away from type aliases and toward interface to allow consumers to adjust/extend types. Interfaces are "open" and type aliases are not. (2) We need to be far more conscientious about what exactly we're exporting (types, values, types & values on the same symbol) so that consumers get what they expect. (2.1) Re-exports should get equal attention to (2), and should have accompanying tests to affirm we support what we need to support, and don't support what we shouldn't support.

[dfreeman]

(1) We need to move away from type aliases and toward interface to allow consumers to adjust/extend types. Interfaces are "open" and type aliases are not.

In the general case I absolutely agree with this, but specifically when we're re-exporting a type defined elsewhere, I think the closed nature of a type alias is a good thing.

By using an interface declaration to re-export a type whose source of truth is elsewhere, the original type and the re-export can diverge. Using an interface for the re-export allows it to be augmented without impacting the definition of the original.

https://www.typescriptlang.org/play/index.html#code/MYGwhgzhAEDiIHsBGYQGEwCdoG8CwAUNMcAopgIQBc0EALpgJYB2A5gNyHHQAmCCmCNWjMArgFskAU0ycCAX0KFmYcVIgAHMMCnQAcqvVaduLsSkAPDQLrRSzetAzYAvHEQp0WOWegB6P2gAFQALXQAzMkQAdxZWaDpohGgQFnU7MGZoaWgWOhlgEMzWKR5oSMxfAOgpAEdRRgA3VClmW2kixsYBaABRAA00XoAFIISi2zowhIBPDV1URkgqwJ5GCFQYmDBRVjU2sDpu5ipfX0trTFs8mXDtXWcai3zmHhh4ZFRHnEUiYmqLjZZvMnFhoG4Pp5nHJfoRqgADWCfLyYeHQTJleEGNSae4AOmcaKwCygjFYKiQIF0dGSCGYUjhgUyCCmMmyUmAOwg1LCMwA5JhdHUGs0qW1gelgKJMIK2iAZgAaRnZUS2aK6TlZJCCsAAa3G62yM3Ru32RzY0CxhlxOgJWHheKUBCptlYHi+WBokI9rlwdjIAhoAtKfIVvH4ghoABZoPI5C6RNbjFJnDRsUZ8Y83G7kdCncok-d9IWTPg-v5AgBVZikcRmuIGmCpel2IpsdKs3hScIsRhHOnQBDhFaW9M2qR21Eq2x6ADyY0R7pR8NOBBHgKuuTatyL32gMyknpEEmksljhHkQA

This can lead to to developer pain and confusion in a codebase that has a mix of e.g. Model and DS.Model annotations, as the two types should be equivalent, but could stop being mutually assignable to one another.

[chriskrycho]

Gah, good writeup guys. I was getting tripped up by the fact that you can reference a type-only item on a namespace, but cannot extract it from a namespace. I.e. this works:

namespace Foo {
  export interface Bar {
    quux: 'hi';
  }
}

export default Foo;

import Foo from 'foo';
let bar: Foo.Bar; // fine

import { Bar } from 'foo'; // doesn't resolve, even if `Bar` is also a value

That latter point is totally expected for all the reasons outlined: namespaces don't work like modules. I have avoided namespaces for so long that I got myself confused about what I was seeing.

---

(1) We need to move away from type aliases and toward interface to allow consumers to adjust/extend types. Interfaces are "open" and type aliases are not.

I'm iffy on this, honestly, for basically the reasons Dan outlines. I'm sensitive to the fact that people often reach for interface merging to deal with ways they're using private API locally, but that always seems smelly to me. Not saying we should block that for sure, but there's a real tradeoff here between "Let people hack round things when they need to" and "Let people mess things up for themselves really badly." 🤔

(Philosophically, I actually disagree quite strongly with the idea that types should in general be "open for extension". I'm aware that makes me weird in OOP-land, however.)

(2) We need to be far more conscientious about what exactly we're exporting (types, values, types & values on the same symbol) so that consumers get what they expect. (2.1) Re-exports should get equal attention to (2), and should have accompanying tests to affirm we support what we need to support, and don't support what we shouldn't support.

100% agreed here.

[mike-north]

Sounds like we have agreement (or no dissent) around (2).

I'll follow up on (1) in more detail when I have more time, but I believe pretty strongly that this is one situation where we should to choose to represent types as accurately as possible rather than as strong as possible.

Let's use Component as an example. Both library authors and consumers expect this to be defined somewhere in ember.js as

export default class Component {}

We could go over to @types/ember and describe this class as follows.

export default class Component {}

This is of course a merged declaration of the Component class or constructor function, and the interface Component which pertains to instances that the factory creates.

Thus, the following is 100% equivalent

interface Component {} // interface for instances
const Component: new () => Component; // value for factory

export default Component; // export as a single declaration-merged symbol

We know that interface Component is what's being used here, because the following works nicely

interface Component {
   foo: string;
}
class Component {
   constructor() {
      this.foo;
   }
}

and this does not

type Component2 = { // 🚨 ERROR Duplicate identifier 'Component2'.
   foo: string;
}
class Component2 {  // 🚨 ERROR Duplicate identifier 'Component2'.
   constructor() {
      this.foo;
   }
}

examples

So here's the question for you @chriskrycho @dfreeman - Are we trying to describe the types for something that's defined in ember.js as class Component {} using the equivalent class-like representation in our .d.ts files, or are we aiming to create something entirely new, and potentially different from what consumers expect?

I look at describing types for a library's class Component {} with anything other than

• class Component {}
• interface Component {}; const Component;

as an inaccurate representation of the underlying entity. Am I missing something here?

[mike-north]

Also another point to consider: we need open-ness in order to define the types themselves. If @types/ember__routing has no knowledge of a DS.Store, we have to augment Route in @types/ember-data

This only works because Route is an interface/class. Things get either more complicated or impossible if we take steps to move these kinds of things to type aliases.

[dfreeman]

@mike-north Just to clarify, I think I'm exactly aligned with you on open-ness and why we want/need it for our declarations. The only case I think it's undesirable is for re-exports, for the reasons described above.

While I agree with @chriskrycho philosophically that it would be nice for that to be unnecessary, pragmatically there are lots of instances in the JS ecosystem where runtime code from one package is augmented by another (like the Route#store example). Trying to avoid that would also be going against the grain of TS itself, since the type declarations generated from a regular class declaration by tsc would be subject to declaration merging.

[mike-north]

@mike-north Just to clarify, I think I'm exactly aligned with you on open-ness and why we want/need it for our declarations. The only case I think it's undesirable is for re-exports, for the reasons described above.

Agreed

Trying to avoid that would also be going against the grain of TS itself, since the type declarations generated from a regular class declaration by tsc would be subject to declaration merging.

Agreed.

Sounds like this is where we are at:

• We should take great care about whether @types/* exports are types, values, namespaces, or some intentional combination of them
• Original declarations should prefer class and interface over type
• Re-exports should be treated as follows
  • Type only: type T = MyInterface;
  • Class: type C = MyClass; const C = MyClass;
  • Value: const V = myValue;
  • Namespace: type N = typeof MyNamespace; const N = MyNamespace;

[chriskrycho]

I think that’s the right answer, yes. I’ve been mulling on my philosophical quibbles for the last hour and find both of your points persuasive. (Also I hate it, but what are you going to do? :laughing:) When I do the re-exports work tomorrow, I’ll use this as the guide; we should also formalize it into guidelines somewhere (in the docs?) for people writing types.

[dfreeman]

Agreed on all counts. I think a page in the docs that's maybe some combination of "specific guidelines for updating @types/ember*" + "general guide for writing type declarations" would be super useful as a resource.

[chriskrycho]

I’m going to be doing at-least-monthly (and probably more than that) work on writing and improving types this summer and fall, and one of the things I’m wanting to drive out of that is actual formalization do what most of us do at a perhaps conscious, perhaps intuitionist, level—but certainly nowhere online clearly articulated—when writing type definitions. That’s a gap for TS in general as well as for our specific community, and getting it turned into some docs people can refer to should be a big win for the whole TS community.

[mike-north]

Since this has been in the way of ember-data's TypeScript conversion and my team is sort of held up (or having to take on tech debt) until this is fixed, I'll take the time to write this up and circulate it amongst typed-ember core.

We should then ensure that PRs opened against @types/{rsvp,ember*,qunit} align with these conventions

[mike-north]

I just ran into an issue relating to this ticket that makes me want to prioritize fixing this.

When trying to obtain the types for from module via dynamic import, the following should be possible.

type Prom<T> = import('rsvp').Promise;

Unfortunately, due to the issue described in this ticket, one has to do the following today

type Prom<T> = InstanceType<typeof import('rsvp').Promise>;

[dfreeman]

While I do still agree that getting crisp about types vs values in our declarations is important, looking at the definitions in DT, the current setup looks correct to me.

Locally, this typechecks as expected for me:

type Prom<T> = import('rsvp').Promise<T>;

I think the issue is the missing type parameter on the RHS?

[mike-north]

Ah, I must have been troubleshooting against an older version of the types (before my https://github.com/DefinitelyTyped/DefinitelyTyped/pull/36942 which adds the interface you linked to).

[chriskrycho]

We took this issue to heart and made sure it was true in DT, and it comes “for free” in Ember’s stable types. Thanks for the discussion (4 years ago now!), all.