Asynchronous app initialization and setup improvements

[daffl]

PR #2255 adds an asyncronous app.setup and service.setup which can be used with new hooks. However, when working with @marshallswain we noticed that this is still not ideal when configuring your application. For example the MongoDB service needs the asynchronous model in the configure function before its setup method is called. We are now looking at adding the following improvements:

• app.configure should allow async functions. They will run in the order app.configure is called and only when the previous function returns. If there is any asynchronous app.configure function, await app.setup() or app.listen() must be called before any services (calling app.service(name)) can be used.
• As an additional improvement and to avoid having the app in an unexpected state (e.g. in tests), if any service implements a setup method, await app.setup() or app.listen() must also be called before any services (calling app.service(name)) can be used.

Both changes will work compatible with existing applications, specifically we wanted to avoid having to unnecessarily call app.setup on client side Feathers apps.

Related to #509 and #853

[marshallswain]

Makes sense to me. To clarify, "using a service" means calling app.service('service-name') to get a reference to the service, right? So calling that before all async setup completes will throw an error?

[daffl]

Correct. I updated the comment. We should be able to make the error message fairly descriptive. In the docs and generated apps we recommend getting the reference as late as possible (e.g. in a specific test instead of storing it early),

[vonagam]

Change to app.configure sounds strange. Right now it is no more than a pretty way to call a function. Which means that there is no difference and expectations are clear.

Now that code will become improper:

app.configure(someConfigure);
app.configure(changeFooSetting);
console.log(app.settings.foo);

The outputted result will depend on whenever someConfigure (which is unrelated to changeFooSetting) is async or not.

Though I understand the need for a way to add async setup steps (wrote and use a simple utility of my own for that). I would recommend to introduce a new app method (addSetup or something) which will collect funcs for running during setup. So there will be clear separation between simple sugar that runs immediately and setup funcs that execute later.

As for mandatory await app.setup() - why ifs? Why not just make it a requirement?

[daffl]

True, having different behavious based on what kind of function is passed (which you may not even know) makes it quite unpredictable.

I'm not totally opposed to making await app.setup() a requirement. Biggest problem is that all the client side code would break (where app.setup() usually doesn't do anything). Maybe configure should stay that way and e.g. the MongoDB service should implement setup where it initializes what it needs and we still change it to require calling await app.setup() if there is any service that has a setup method.

[erezbens]

I encountered a problem when trying to init Sequelize service.
Found a way around it by splitting the generated code from feathers generate service (in sequlize.js) into two parts, and place app.configure(services) in between, like this:

const connectionString = app.get('postgres');
const sequelize = new Sequelize(connectionString, {
  dialect: 'postgres',
  logging: false,
  define: {
    freezeTableName: true,
  },
});

app.set('sequelizeClient', sequelize);

app.configure(services);

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

[vonagam]

Another thing about second idea (throwing on service() before setup()): It will mean that you cannot add hooks to services before setup() is called, since service() is the only proper way to get a real service instance with proto and mixins applied.

[daffl]

This has been addressed in #2585 as setup and teardown hooks

[RuneSP]

I'm getting this error when trying to use a setup hook:

Error: Can not apply hooks. 'setup' is not a function
[start:*run]     at ..\node_modules\@feathersjs\hooks\script\hooks.js:66:19

My code in app.ts

app.hooks({
    setup: [
        async function (context, next) {

        }
    ]
})

package.json

  "dependencies": {
    "@feathersjs/configuration": "^5.0.0-pre.18",
    "@feathersjs/express": "^5.0.0-pre.18",
    "@feathersjs/feathers": "^5.0.0-pre.18",
    "@feathersjs/socketio": "^5.0.0-pre.18",
    ...
}

[daffl]

Ah, this only happens with Express apps because they are technically a function. Will be fixed in the next version via the linked pull request.

[RuneSP]

Ok, I dont think I actually need express anyway as I'm running everything through socketio. I dont get the same error now when not using express, but it seems context.app is undefined in this case:

const app: Application = feathers();
app.configure(configuration());                    
app.configure(socketio);
app.configure(mongoose);
app.configure(customModule);
app.configure(services);
app.configure(channels);

app.hooks({
    setup: [
        async (context: ApplicationHookContext, next) => {
            console.log(context.app); // undefined
            await app.get('customModule').init();
            await next();
        }
    ],
})

Also, if I'm passing any of the normal service hooks (before etc) in the same object as setup / teardown, I'm getting an invalid method error.

[FossPrime]

This is also related to #1965 https://stackblitz.com/edit/feathers-vite-chat?file=src%2Fservices%2Findex.ts

App.ts in the feathers-chat app is completely synchronous. The discoverability of this feature is not great. To improve the DX I think every service configure and every app.ts should be async by default. Top-level await support is not great. Top level function calls in modules is also problematic with HMR and general DX, even if you're not a fan of classy Javascript.

In general running code top level often pollutes globals and pseudo-globals, and as such should be avoided. Imagine copying App.ts renaming it and importing it to index.ts, next to the original app.ts. That would cause chaos. Yes I know you can alias package names in package.json, but even that would solve all the global conflicts server side. I just want to forget the fs.readFileSync exists, aswell as DRY up service initialization for my needs without deleting all the code and starting over with every prototype.

ASYNC ALL THE THINGS... agreed?

Async playground https://stackblitz.com/edit/feathers-async-playground?file=src%2Fservices%2Findex.ts