typed-ember / ember-cli-typescript

Use TypeScript in your Ember.js apps!
https://docs.ember-cli-typescript.com
MIT License
363 stars 99 forks source link

Documentation: Recommended return type of synchronous hasMany relationships is wrong #1449

Open mspoehr opened 3 years ago

mspoehr commented 3 years ago

According to ember-cli-typescript's ember-data Model documentation, the type of synchronous has many relationships is EmberArray<T extends Model>:

The type returned by the @hasMany decorator depends on whether the relationship is { async: true } (which it is by default).

  • If the value is true, the type you should use is DS.PromiseManyArray<Model>, where Model is the type of the model you are creating a relationship to.
  • If the value is false, the type is EmberArray<Model>, where Model is the type of the model you are creating a relationship to.

This is incorrect. According to Ember 3.28's documentation:

In contrast to async relationship, accessing a sync relationship will always return a ManyArray instance containing the existing local resources. But it will error on access when any of the known related resources have not been loaded.

This makes sense because it would mean that async relationships return a type of PromiseManyArray<Model>, while sync relationships return ManyArray<Model>. Ember-cli-typescript's types for DS even define this:

type AsyncHasMany<T extends Model> = PromiseManyArray<T>;
type SyncHasMany<T extends Model> = ManyArray<T>;

I've updated my typings of sync relationships to MutableArray<Model> instead of EmberArray<Model>, as ManyArray extends MutableArray and I don't want to import DS. Is this the correct approach? My recommendation would be to update the documentation to reflect this approach.


Small code example to drive my point home:

import Model, { hasMany } from '@ember-data/model';
import EmberArray from '@ember/array';
import Process from './process';

export default class Thread extends Model {
  @hasMany('process', { async: false })
  declare processes: EmberArray<Process>

  addProcess(process: Process) {
    this.processes.addObject(process);
  }
}

With EmberArray<Process>, addObject has a type error:

Property 'addObject' does not exist on type 'Array<Process>'.

However, the code does work if typechecking is disabled, and there are no type errors when using processes: MutableArray<Process> from @ember/array/mutable.

chriskrycho commented 3 years ago

Thanks for the report! This makes sense; Ember Data has made a lot of progress and the types have changed a lot over the 3.x series (in ways which are backwards compatible, to be clear), and we haven't kept up!