feathersjs-ecosystem / feathers-objection

Feathers database adapter for Objection.js, an ORM based on KnexJS SQL query builder for Postgres, Redshift, MSSQL, MySQL, MariaDB, SQLite3, and Oracle. Forked from feathers-knex.
MIT License
98 stars 49 forks source link

Examples of relations in the docs are wrong for TS #162

Open Artaud opened 2 years ago

Artaud commented 2 years ago

Hi, I've been setting up a new Feathers app with ObjectionJS and Typescript recently and found out that if I set up relations according to the examples outlined in the docs here (on the feathers-objection lib), it works initially - but once you start doing something less basic it falls apart. I think I have a correct way to set it up so if interested, I'll do a PR.

Unfortunately it's going to need a PR in the feathers generator as well but it's not a big change so I'm keen on doing that as well if approved here.

The main issue is that the docs here suggest doing the following for setting up relations:


class User extends Model {
  static get tableName() {
    return 'user';
  }
  ...
}

module.exports = function(app): typeof User {
  if (app) {
    const db = app.get('knex');

    db.schema
      .hasTable('user')
      .then(exists => {
        if (!exists) {
          db.schema
            .createTable('user', table => {
              table.increments('id');
            })
        }
      })
  }

  return User;
};

The problem is that in TS with the default export you can't return type "User" but have to return "typeof User" since it's not an instance but a class definition you're returning.

So I propose to also export the model class itself and drop the if (app) check, so you'll use the default exported function only when creating the Feathers service, but use the model export whenever you need to reference it e.g. when setting up relations.

dekelev commented 2 years ago

It would be better if you can open a PR, because I'm not sure that I fully understand what you're looking for. I'm not sure if you need to change the lib code or to add another example of service & model initialization that works better with TypeScript.

בתאריך יום ו׳, 19 בנוב׳ 2021, 14:51, מאת Jiří Richter ‏< @.***>:

Hi, I've been setting up a new Feathers app with ObjectionJS and Typescript recently and found out that if I set up relations according to the examples outlined in the docs here (on the feathers-objection lib), it works initially - but once you start doing something less basic it falls apart. I think I have a correct way to set it up so if interested, I'll do a PR.

Unfortunately it's going to need a PR in the feathers generator as well but it's not a big change so I'm keen on doing that as well if approved here.

The main issue is that the docs here suggest doing the following for setting up relations:

class User extends Model { static get tableName() { return 'user'; } ... }

module.exports = function(app): typeof User { if (app) { const db = app.get('knex');

db.schema
  .hasTable('user')
  .then(exists => {
    if (!exists) {
      db.schema
        .createTable('user', table => {
          table.increments('id');
        })
    }
  })

}

return User; };

The problem is that in TS with the default export you can't return type "User" but have to return "typeof User" since it's not an instance but a class definition you're returning.

So I propose to also export the model class itself and drop the if (app) check, so you'll use the default exported function only when creating the Feathers service, but use the model export whenever you need to reference it e.g. when setting up relations.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/feathersjs-ecosystem/feathers-objection/issues/162, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABB5E3IQTD7MRMRFB7E7THTUMZB5BANCNFSM5IMCHZOA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Artaud commented 2 years ago

It's just for docs here. I'll do the PR at the start of the next week.