Suggestion: BaseModel configuration prop

[sidferreira]

Proposal

Add a BaseModel to host static methods for models

How it could work

• Add optional config BaseModel
• If BaseModel is declared, as soon as the database has a handle, it would BaseModel.database = database, even before declaring collectionMap

Example

I have 32 model classes atm, and for collection methods I decided to use Model's static methods. For example:

Post.find() is basically: database.collections.get('posts').find() but In a way nicer way.

Instead of just adding it to each class, I created a class BaseModel extends Model and all my classes extend this BaseModel.

So I added in Base Model methods like:

static getCollection<T extends typeof Model>(
    this: T,
  ): Collection<InstanceType<T>> {
    const database = ((this as unknown) as BaseModel).database;
    return database.get<InstanceType<T>>(this.table);
  }

static async find<T extends typeof BaseModel>(this: T, id: string) {
    let item: InstanceType<T> | undefined;

    try {
      item = await this.getCollection().find(id);
    } catch (e) {
      this.registerNotFound(id);
    }

    return item;
  }

  static async findBy<T extends typeof BaseModel>(
    this: T,
    fieldName: string,
    value: string,
  ) {
    let item: InstanceType<T> | undefined;
    let itemArray = await this.fetchBy(fieldName, value, Q.experimentalTake(1));

    try {
      item = itemArray[0];
    } catch (e) {
      this.registerNotFoundBy(fieldName, value);
    }

    return item;
  }

  static findAndObserve<T extends typeof BaseModel>(this: T, id: string) {
    return this.getCollection().findAndObserve(id);
  }

This allows me to write cleaner code, for example in observables:

export const withPostsForHome = withObservables([], () => ({
  posts: Posts.postsForHome(),
}));

The only issue is that I set the BaseModel database way after the database is ready, having some errors once in a while, especially when hot reloading a model. So I'm suggesting this change in the hope I reduce (or zero) the current issues.

If agreed, I would love to write the changes but I will need help with typing

[sidferreira]

cc @radex

[radex]

sorry, but it's a no from me. This was considered and rejected, oh, like 4 years ago. Posts.foo() is nice, but it's not clean. I refuse to promote a coding practice that implicitly make a Database a singleton. You should be able to have multiple instances of a Database, of a Collection, etc., and this proposal prevents that.

What we practice at Nozbe is that there's a ModelRoot instance that contains methods that act on the whole database (when there is no Model instance where the method fits) - e.g. your postsForHome would be there. This object is available the same way as a Database:

const { modelRoot, db } = useServices()

[sidferreira]

Indeed one point that I was wondering was about the singleton issue.

I believe the only workaround is having the database being an argument for such class methods.

On the other hand, the modelRoot doesn't quite solve our issue as we wanna use the database in sagas and etc.

I guess the modelRoot would be nice example on Pro Tips...

[radex]

On the other hand, the modelRoot doesn't quite solve our issue as we wanna use the database in sagas and etc.

Sorry, I'm not sure what that means, can you elaborate?

I guess the modelRoot would be nice example on Pro Tips...

yes

[sidferreira]

@radex what if we do such replacements on Collection, suggesting the usage of database.collections.get('posts')?

Important to mention, IMHO, that I believe Watermelon should be a bit more "decoupled", maybe not the best wording, but for example:

  async update(recordUpdater: (this) => void = noop): Promise<this> {
    this.collection.database._ensureInAction(
      `Model.update() can only be called from inside of an Action. See docs for more details.`,
    )
    const record = this.prepareUpdate(recordUpdater)
    await this.collection.database.batch(this)
    return record
  }

it accesses both collection and database, in a way that any change in this process by collection or database would require a full rewrite here.

On the other hand, we could have something like

// Model
  async update(recordUpdater: (this) => void = noop): Promise<this> {
    const record = await this.collection.updateRecord(recordUpdater)
    return record
  }
// Collection
  async updateRecord(recordUpdater: (this) => void = noop): Promise<this> {
    this.database._ensureInAction(
      `Collection.updateRecord() can only be called from inside of an Action. See docs for more details.`,
    )
    const record = this.prepareUpdate(recordUpdater)
    await this.database.batch(this)
    return record
  }

[radex]

what if we do such replacements on Collection

I don't immediately hate the idea - it seems logical that if a Model can be extended with methods for a specific type of a model, then a Collection can be extended with methods for a specific type of a collection.

What I'm very concerned about is typing. I would be very hesitant to make such a big, important API change if database.collections.get('posts') can't be inferred to be a specific subclass of a Collection. Do you have some more specific thoughts on how you'd see this API?

it accesses both collection and database, in a way that any change in this process by collection or database would require a full rewrite here. On the other hand, we could have something like...

can you elaborate here? what is being gained by this rewrite exactly? Collection would have to call Model anyway

[ortonomy]

@radex -- can you share this modelRoot implementation, as I've seen you mention it a few times, but I don't see how it can be constructed.

[sidferreira]

@radex About typing, I was thinking about const database = new Database<BaseCollectionClassType>(...).

[stale[bot]]

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.