Add relationship interfaces and deprecate bunch of cache/model methods

[tchak]

This PR is adding a Relationship interface. It is similar to ember-data Reference interface. The mane goal is to provide a more consistent and smaller API surface.

class Model {
  hasOne(relationship: string): HasOneRelationship;
  hasMany(relationship: string): HasManyRelationship;
}

class Relationship {
  readonly name: string;
  readonly owner: Model;
}

class HasOneRelationship extends Relationship {
  get value(): Model | null;

  query(options?: object): Promise<Model | null>;

  replace(record: RecordIdentity | null, options?: object): Promise<void>;
}

class HasManyRelationship extends Relationship {
  get value(): Model[];
  get ids(): string[];

  query(options?: object): Promise<Model[]>;

  add(record: RecordIdentity, options?: object): Promise<void>;
  remove(record: RecordIdentity, options?: object): Promise<void>;
  replace(records: RecordIdentity[], options?: object): Promise<void>;
}

[tchak]

This PR also reduces usage of ArrayProxy to LiveQuery. HasMany relationships are now regular arrays. We might want to freeze them in development environment.

[dgeb]

@tchak the relationship interfaces are extremely promising!

I wonder if it would feel more natural to access planet.relationships.moons.value rather than planet.hasMany('moons').value.

HasMany relationships are now regular arrays.

How are changes in the associated cache used to invalidate these arrays? We of course need to ensure that {{#each planets.moon as |moon|}} remains reactive as the related recordset mutates.

[tchak]

@dgeb I don't understand your example. If planets is a has many what planets.moon means?

[dgeb]

@tchak Sorry, I misplaced the s! I just meant planet.moons.

[tchak]

Well this will completely work because we invalidate planet.moons when it changes.

[dgeb]

I was thinking about how we invalidate each liveQuery but now that I'm reviewing the code I see that we invalidate the relationships via https://github.com/orbitjs/ember-orbit/blob/master/addon/-private/cache.ts#L361-L365

Sorry, I should have just re-checked that logic in the first place!

Anyway, let me do a more in depth review of this PR now ...

[tchak]

I wonder if it would feel more natural to access planet.relationships.moons.value rather than planet.hasMany('moons').value.

The main reason I prefer planet.hasMany('moons') over planet.relationships.moons is because I think the first will be more straight forward to type. Prior art exists in objection.js where they use generics: planet.hasMany('moons')<Moon>. I wouldn't mind it being planet.relationship('moons') if you prefer, but again, typing will be cleaner with distinction between hasMany and hasOne.

[dgeb]

I can see how the distinction is useful for typing, and it's one we already make for query expressions such as FindRelatedRecord and FindRelatedRecords.

For consistency with those query expressions, I wonder if planet.relatedRecords('moons') and moon.relatedRecord('planet') might be a slight improvement?

I'm also wondering whether Relation might be better Relationship? A "relationship" could be considered the definition for all models of a type (i.e. the schema), while a "relation" is the individual instance of a relationship for a given record.

Here is an alternative naming proposal to discuss:

class Model {
  relatedRecord(relationship: string): RecordRelation;
  relatedRecords(relationship: string): RecordsRelation;
}

class Relation {
  readonly name: string;
  readonly owner: Model;
}

class RecordRelation extends Relation {
  get value(): Model | null;
  get id(): string | null;

  query(options?: object): Promise<Model | null>;

  replace(record: RecordIdentity | null, options?: object): Promise<void>;
}

class RecordsRelation extends Relation {
  get value(): Model[];
  get ids(): string[];

  query(options?: object): Promise<Model[]>;

  add(record: RecordIdentity, options?: object): Promise<void>;
  remove(record: RecordIdentity, options?: object): Promise<void>;
  replace(records: RecordIdentity[], options?: object): Promise<void>;
}

[dgeb]

There seems to be a good opportunity to align with #241 here:

store.records('planet').query();
store.records('planet').peek();
planet.relatedRecords('moons').query();
planet.relatedRecords('moons').peek();

And even:

store.records('planet').live();
planet.relatedRecords('moons').live();

[tchak]

I agree on Relation.

I think relatedRecord/relatedRecords will be confusing as it returns a Relation not a record. What about hasManyRelation/hasOneRelation?

[tchak]

So maybe:

class Model {
  hasOneRelation(relationship: string): HasOneRelation;
  hasManyRelation(relationship: string): HasManyRelation;
}

or

class Model {
  recordRelation(relationship: string): RecordRelation;
  recordsRelation(relationship: string): RecordsRelation;
}

[tchak]

hmm, I see what you suggesting with aligning store and record interfaces. I think there is a useful distinction between:

store.findRecords('planet').peek(); // query the cache

and

store.peekRecords('planet'); // get raw cache content

The second is equivalent to store.cache.peekRecords('planet') so I guess we could get away with store.records('planet).peek() and store.cache.records('planet')

[tchak]

class Store {
  record<M = Model>(identifier: RecordIdentity): RecordQueryBuilder;
  records<M = Model>(identifier: RecordIdentity): RecordsQueryBuilder;
  cache: Cache;
}

class Cache {
  record<M = Model>(identifier: RecordIdentity): M | undefined;
  records<M = Model>(identifier: RecordIdentity): M[] | undefined;
  relatedRecord<M = Model>(identifier: RecordIdentity, relationship: string): M | null | undefined;
  relatedRecords<M = Model>(identifier: RecordIdentity, relationship: string): M[] | undefined;

  has(identifier: RecordIdentity): boolean;
  raw(identifier: RecordIdentity): Record | undefined;
}

class QueryBuilder<T> {
  filter(): this;
  sort(): this;
  page(): this;

  raw(): this<Record>;

  query(): Promise<T>;
  peek(): T;
}

class RecordQueryBuilder<M> extends QueryBuilder<M> {
}

class RecordsQueryBuilder<M> extends QueryBuilder<M[]>  {
  live(): this;
}

class Model {
  relatedRecord<M = Model>(relationship: string): RecordRelation;
  relatedRecords<M = Model>(relationship: string): RecordsRelation;
}

class Relation<O, T> extends QueryBuilder<T> {
  readonly name: string;
  readonly owner: O;
}

class RecordRelation<O, M> extends Relation<O, M | null> {
  get value(): M | null;
  get id(): string | null;

  replace(record: RecordIdentity | null, options?: object): Promise<void>;
}

class RecordsRelation<O, M> extends Relation<O, M[]> {
  get value(): M[];
  get ids(): string[];

  live(): this;

  add(record: RecordIdentity, options?: object): Promise<void>;
  remove(record: RecordIdentity, options?: object): Promise<void>;
  replace(records: RecordIdentity[], options?: object): Promise<void>;
}

So this is what I have so far:

store.records('planet').filter().query()
store.records('planet').filter().peek()
store.records('planet').filter().live().query()
store.records('planet').filter().raw().peek()

and

planet.relatedRecords('moons').filter().peek()
planet.relatedRecords('moons').filter().query()
planet.relatedRecords('moons').filter().live().peek()
planet.relatedRecords('moons').value
planet.relatedRecords('moons').add()

This looks good, but I don't really like the mix of QueryBuilder and Relation interfaces. I feels wired. One could do:
planet.relatedRecords('moons').filter().add()
We could assert or type it in a more advanced way. I don't know...

[tchak]

I guess we could do :

planet.relatedRecords('moons').filter().peek()
planet.relatedRecords('moons').filter().query()
planet.relatedRecords('moons').filter().live().peek()
planet.recordsRelation('moons').value
planet.recordsRelation('moons').add()

[tchak]

superseded by #278