rfc: ember-data | Deprecate Legacy Imports

[runspired]

Full Text: Rendered

[SergeAstapov]

@runspired as this deprecation targets 5.0, I assume PromiseObject and PromiseManyArray would be removed as well?

Calling these two specifically because needed for TypeScript users per https://docs.ember-cli-typescript.com/ember-data/models#importing-promiseobject-and-promisemanyarray

[runspired]

@SergeAstapov I'm surprised @types/ember-data has not made an effort to provide these types. Probably a good place for @chriskrycho to weigh in. Potentially making these types named exports in the model package within @types/ember-data is the path forward for those users, though more likely I would think that the types for hasMany and belongsTo could be made to adequately work for this without the need to an import. I must be missing some nuance.

EDIT

Seems the nuance is that typescript expects a void or any return type from decorator functions and does not provide a mechanism for a decorator to describe the value. So for instance something like this would not work to set the type for a belongsTo.

type Proxy<T> = {
  [K in keyof T]: T[K];
}

type PromiseRecord<T> = Proxy<Promise<T> & T> & { content?: T };

interface decorate<T> {
  (target: T, propertyKey: string, descriptor: PropertyDescriptor): PropertyDescriptor & { value: T };
}

export function belongsTo<T>(type: string): decorate<PromiseRecord<T>>;
export function belongsTo<T>(type: string, options: {}): decorate<PromiseRecord<T>>;
export function belongsTo<T>(type: string, options: { async: true }): decorate<PromiseRecord<T>>;
export function belongsTo<T>(type: string, options?: { async: false }): decorate<T>;
export function belongsTo<T>(type: string, options?: { async?: boolean }): decorate<PromiseRecord<T> | T>;

This said this is roughly how @types/ember-data could provide a nice type for these proxies on their own.

[chriskrycho]

The reason we haven't provided imports for them in a new location is that we have a policy of only providing imports for actual public API from Ember. That is: If there is no public export for something, we don't supply a public export type for it either. The types themselves aren't complicated (though they'd probably look a little different from the above); it's just that providing an import Ember itself doesn't could be quite confusing to users.

[runspired]

It seems clear that for typescript support to be first class within the ember ecosystem that at some point libraries will have to expose type info for private classes that don't have public imports. EmberData has a lot of these in addition to these proxies. Things like Snapshot and Reference which are not mean to be user importable but for which instances are absolutely public API.

I think the main problem here though is that decorators don't prove typescript type info. These particular types only "need" an export because the person defining the ember-data model must make use of them to assign a type to the value. That is an action which unfortunately we cannot do.

I would not want to hold on to exports of these things just for typescript usage though. I'd love to see some RFC design work around what this might look like in a world with full TS support from ember, but in the mean time I think either @types/ember-data or or another project should probably ship something along the lines of ember-data-internal-types.

Since typescript is happy to import types from locations in the internals that are not actually importable in the final built output (since we import rollup) – a strategy we utilize in EmberData itself as we have been converting to TS, I'd be happy to consider that once ember-data has official typescript support there be either a typescript specific package for providing these typings or a contract that a set of non-runtime-importable paths be considered public for type information. We're pretty far from that point though currently.

[chriskrycho]

Yes! My own take here is that users should be able to import and name types like these as public API, but the public API import will be exactly and only that: the ability to name the type for things like this, passing the result to a well-typed function, and so on. The only reason we haven't shipped something like that already is a desire to avoid churn. We may need to address it with a stopgap depending on the timelines of this deprecation landing and when official TS support ships, which should be fine if needed. We'll just do the extra work and deal with the churn at that point.

[igorT]

@runspired to go through the existing internal exports to figure out what we would be leaving behind.

[muziejus]

Regarding "how we teach this," I recommend actively changing the ember-cli-typescript docs (at least until first-class TS lands in ember/ember-data). Mostly fleshing this out:

There is no public import path in the Ember Data Packages API for the PromiseObject and PromiseManyArray types. These types are slowly being disentangled from Ember Data and will eventually be removed. However, until they are, we need a way to refer to them. For now, the best option is to refer to them via the legacy DS import.

I also recommend getting in touch with @gitkrystan to change the Skylight blog post about TS in Ember to reflect the changes, as it's the second most appropriate thing to come up when I google "Ember Typescript."

Third, following @gitkrystan's lead, I've building a repo of a "typescripted" version of the ember super rentals tutorial. I recommend incorporating something like that into the ember-cli-typescript docs too, in preparation of this.

Finally, there should be a page on the ember-cli-typescript docs page dedicated explicitly to dealing w/ deprecated legacy imports.

These sources don't reflect the current state of codemods/linting, I don't think? Apologies if they do.

[wagenet]

@runspired is there anything blocking this from being accepted?

[runspired]

The 2022 Update to the TS guide/blogpost and current ember-data TS types in the @types project both have the alternative to using DS. https://blog.skylight.io/ts-extends-confidence-2-2022/

import Model, {
  AsyncBelongsTo,
  AsyncHasMany,
  belongsTo,
  hasMany,
} from '@ember-data/model';
import Comment from 'my-app/models/comment';
import User from 'my-app/models/user';

export default class PostModel extends Model {
  @belongsTo('user') declare author: AsyncBelongsTo<User>;
  @hasMany('comments') declare comments: AsyncHasMany<Comment>;
}

[wagenet]

@runspired ah yeah, I haven’t actually been reading all of these in depth. Do you think we should still try to merge? Or just close at this point?

[runspired]

@wagenet just the opposite, the only opposition here was that community typescript projects were doing bad things, they have a path away and learning materials were updated with that path so I think we can proceed here.

[chriskrycho]

And with my TS hat on: we all agree that it was bad! Everyone is well aligned that it was a case of “recommended bad workarounds because there want a better one; now there’s a better one, let’s go!”