Sequelize file should not alter app.setup

[steve-kaufman]

The sequelize.js module generated by the feathers cli modifies the app.setup method to include setting up the database relations followed by the sequelize.sync() call:

const oldSetup = app.setup

app.set('sequelizeClient', sequelize)

app.setup = function (...args) {
  const result = oldSetup.apply(this, args)

  // Set up data relationships
  const models = sequelize.models
  Object.keys(models).forEach(name => {
    if ('associate' in models[name]) {
      models[name].associate(models)
    }
  })

  // Sync to the database
  app.set('sequelizeSync', sequelize.sync())

  return result
}

Apart from this being somewhat convoluted, it raises problems when testing. Most of the documentation surrounding unit testing assumes that the backend is NeDB, and I found virtually no resources explaining how to go about testing services with any other backend.

The issue here is that sequelize.sync is quite the misnomer, as it is in fact an asynchronous method. If I'm using a sequelize backend (which I am), and I want to test a service (which I do), then I need to manually call app.setup() (to create the associations) and then await app.get('sequelizeClient').sync(), otherwise the tables may be undefined (assuming the testing database is separate from the development/production database). What's more is that app.setup() calls sequelize.sync itself, but it simply doesn't await it, rendering it ineffective in a testing environment where I want to immediately make calls to the services, and I end up calling sequlize.sync twice.

Now the workaround is simple. I just commented out the line in sequelize.js that calls sequelize.sync, and I add my app.setup() and await app.get('sequelizeClient').sync() to a beforeAll in my test suite. However, it was annoyingly difficult to figure out why the tables were nonexistent, and I'm a little surprised that the documentation largely ignores sequelize.

I would love to see the documentation for testing extended to include examples with other database adapters, but in the meantime I think it would be good to rethink how app.setup is oddly just rewritten here.

• Using @feathersjs/cli version 4.3.0

Edit:

It appears I should've opened this issue in generator-feathers instead

[daffl]

Currently this can be addressed as mentioned in https://github.com/feathersjs-ecosystem/feathers-sequelize/issues/310#issuecomment-514067510:

const app = require('../src/app');

const setupApplication = async () => {
  app.setup();
  return app.get('sequelizeSync');
};

module.exports = {
  async up () {
    await setupDb();
    // Do things here
  },

  async down () {
    await setupDb();
    // Do things here
  }
};

In the upcoming version, app.setup() will be asynchronous and in newly generated applications .sync() will no longer be called and instead encourage the proper use of migrations.

[steve-kaufman]

Sounds great! I just found myself frustrated trying to figure out why the tables were undefined in my tests, and the way the database synchronization gets abstracted into app.setup confused me further.

I thought it would be a good idea to put an issue out there, because this definitely left me scratching my head trying to figure out what was happening asynchronously. I mean, whose idea was it to name an asynchronous method sync anyway :)

[daffl]

v5 now comes with setup and teardown hooks which can address this issue. By default it also uses the more low level Knex as the default SQL driver which combines very well with the new @feathersjs/schema