feathersjs/hooks

Async middleware for JavaScript and TypeScript

hooks's Issues

Result and undefined

To check if result is set and if a function should be skipped an equality to undefined is used. But what if one wants to set result to undefined as a real return value?

Maybe "result" in ctx check should be used instead as a more strict rule? (And if you need to remove already set result from previous hooks, you'll do delete ctx.result.)

(Just a thought, nothing more.)

Is it possible to remove a hook?

For example, I have the following:

const { hooks } = require('@feathersjs/hooks');

const logRuntime = async (context, next) => {
	// ...
}

class Hello {
  async sayHi (name) {
    return `Hi ${name}`
  }
}

hooks(Hello, {
  sayHi: [
    logRuntime
  ]
});

How do I remove the logRuntime hook?

API usage improvements

The current API still feels a little verbose. I'd really like to offer something that is chainable, offering all the current functionality but a little more concise:

const sayHi = hooks(name => `Hi ${name}`, [ hook1 ]).params('name').defaults({
  name: 'Dave'
});

This would still work on objects:

const o = {
  sayHi(name) { return `Hi ${name}` }
}

hooks(o, {
  sayHi: hooks([ hook1 ]).params('name').defaults({
    name: 'Dave'
  })
});

Error hooks - re-throwing vs assigning context.error

The behaviour of throwing in an error hook changed slightly from V4->V5.

If an error hook modifies context.error - that's all fine, the context is now updated and the error is re-thrown in regular.ts.
But if an error hook throws an error, that does not update context.error like it used to.

Perhaps this here should .catch(error => { context.error = error; throw context.error })

hooks/main/hooks/src/regular.ts

Lines 52 to 56 in 2490de6

	return runHook(hook, context, 'error').then(() => {
	if (context.result === undefined && context.error !== undefined) {
	throw context.error;
	}
	});

?

Performance optimizations

One of the beta testers reported a noticeable performance difference in the latest version vs the current Feathers hooks. I ran a simple benchmark with different configurations and got the following results:

Baseline 11
Empty hooks 115
Single simple hook 138
Single hook and withParams 813
Single hook, multiple withParams 1331

Using the following test code:

  it.only('benchmark', async () => {
    const getRuntime = async (callback: () => Promise<any>) => {
      const start = Date.now();
    
      for (let i = 0; i < 100000; i++) {
        await callback();
      }
    
      return Date.now() - start;
    }
    const hello = async (name: string, _params: any = {}) => {
      return `Hello ${name}`;
    };
    console.log('Baseline', await getRuntime(() => hello('Dave')));

    const hookHello1 = hooks(hello, []);
    console.log('Empty hooks', await getRuntime(() => hookHello1('Dave')));

    const hookHello2 = hooks(hello, [
      async (_ctx: HookContext, next: NextFunction) => {
        await next();
      }
    ]);

    console.log('Single simple hook', await getRuntime(() => hookHello2('Dave')));

    const hookHello3 = hooks(hello, {
      middleware: [
        async (_ctx: HookContext, next: NextFunction) => {
          await next();
        }
      ],
      context: [withParams('name')]
    });

    console.log('Single hook and withParams', await getRuntime(() => hookHello3('Dave')));


    const hookHello4 = hooks(hello, {
      middleware: [
        async (_ctx: HookContext, next: NextFunction) => {
          await next();
        }
      ],
      context: [withParams(), withParams('name'), withParams()]
    });

    console.log('Single hook, multiple withParams', await getRuntime(() => hookHello4('Dave')));
  });

While there will be always some kind of overhead of course, I think runtime could be improved by wrapping the context updaters into a single function instead of looping every time.

ERROR in ./node_modules/@feathersjs/hooks/esm/base.js 55:38

ERROR in ./node_modules/@feathersjs/hooks/esm/base.js 55:38
Module parse failed: Unexpected token (55:38)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|     }
|     middleware(middleware) {
>         this._middleware = middleware?.length ? middleware : null;
|         return this;
|     }

how fix it?

Notes on regular hooks support

1. fromErrorHooks vs .map(fromErrorHook):
Initially there were only fromErrorHooks which were backwards compatible with feathers 4 error hooks - they all run in a single catch() call, so if one of them throws too then others do not run. I added fromErrorHook as a utility that person can use to get a single catch() hook. So if you do .map(fromErrorHook) then you get a bunch of catch() calls, not one as before.

I'm ok with this breaking change, but I assume that you are not and that it might have been overlooked. (If it was deliberate decision than all is good.)

2. Who should go first afterHooks or beforeHooks?
Small thing, but I think it is better for beforeHooks to run before afterHooks since there is no point to call an after hook if a before hook throws, it will just add noise to a trace. (I'm talking about this line.)

3. type should be required in runHook internal util:
It was optional for fromHooks and required for fromHook since there are no fromHooks now, type should be non-optional argument and if (type) can be simply removed. (This of course depends on the resolution for first point, if fromHooks will be back than this one needs no change.)

4. Propagation of bad practice with support for returning {...context}:
As I said in #1443 and #2462 supporting for returning new context object from a regular hook is a wrong choice (no upside beside backwards compatibility for obscure feature and a lot of downside). Now it tries to migrate from feathers into hooks. Additional example for why it is not right way: in a hook manager there is props() method with which you can set additional properties on a context, but props() does not simply Object.assign passed properties, it copies property descriptors, allowing for readonly and getters/setters things. Support for practice of {...context} goes against this, since all of those descriptors will be lost and will lead to bugs when those props get used before a cloned context is returned. (Thought about it when was looking how to migrate context.statusCode into context.http.statusCode with getters/setters.)

Critical dependency: require function is used in a way in which dependencies cannot be statically extracted

Hi 👋
I'm trying to compile ts library with hooks package, but keep getting error issues which point to umd module.

WARNING in ../../node_modules/@feathersjs/hooks/umd/utils.js 3:24-31
Critical dependency: require function is used in a way in which dependencies cannot be statically extracted
 @ ../../node_modules/@feathersjs/hooks/umd/base.js
 @ ../../node_modules/@feathersjs/hooks/umd/index.js
 @ ./src/plugins/ImageColorConverter/ImageColorConverter.ts 55:14-42
 @ ./src/index.ts 6:28-88

I have several issues like this. This is my webpack.config file that I'm using

const path = require('path')

module.exports = {
  entry: './src/index.ts',
  output: {
    path: path.resolve(__dirname, 'dist'),
    filename: 'index.js',
    library: {
      name: 'productDesigner',
      type: 'umd',
    },
  },
  module: {
    rules: [
      {
        test: /\.tsx?$/,
        loader: 'ts-loader',
        exclude: /node_modules/,
      },
    ],
  },
  resolve: {
    extensions: ['.ts', '.tsx', '.js'],
  },
  externals: {
    react: 'react',
    lodash: 'lodash',
  },
}

I'm using in in one submodule of this library.

Module versions
"@feathersjs/hooks": "0.7.2"

NodeJS version:
v16.13.0

Operating System:
macOS Monterey

Browser Version:
Chrome Version 97.0.4692.99 (Official Build) (arm64)

Module Loader:
"webpack": "5.68.0",

function.length is lost

function.length is lost, and I see an error calling hooks without hooks argument.

const { hooks } = require('@feathersjs/hooks');

const emptyHook = async (context, next) => next();

// Hooks can be used with a function like this:
const sayHello = async name => {
  return `Hello ${name}!`;
};

const sayHello2 = hooks(async name => {
  return `Hello ${name}!`;
});

const sayHello3 = hooks(async name => {
  return `Hello ${name}!`;
}, []);

const sayHello4 = hooks(async name => {
  return `Hello ${name}!`;
}, [emptyHook]);

(async () => {
  console.log(sayHello.length); // 1
  console.log(sayHello2.length); // 3, should be 1
  console.log(sayHello3.length); // 0, should be 1
  console.log(sayHello4.length); // 0, should be 1
  
  console.log(await sayHello('Bertho'));
  // console.log(await sayHello2('Bertho'));
  // Throw an error: TypeError: manager.parent is not a function
  //  at Object.setManager
  console.log(await sayHello3('Bertho'));
  console.log(await sayHello4('Bertho'));
})();

Combining multiple middleware with compose

This part is not documented on @feathersjs/hooks: https://github.com/koajs/koa/blob/master/docs/guide.md#combining-multiple-middleware-with-koa-compose

const { hooks, compose } = require('@feathersjs/hooks');

hooks(Hello, {
  sayHi: [
    compose([
      logRuntime,
      validateName,
      compose([
        /* ... */
      ])
    ])
  ]
});

Try it: https://runkit.com/embed/l7srsn9s0s2q

Version 0.7.x fails to compile in Webpack

TL;DR

Version 0.7.x fails to compile (Webpack). Version 0.6.5 compiles successfully.

I tried 0.6.5 after finding issue #98

Steps to reproduce

I have a generic VueJS 2 install created with the Vue CLI (using Webpack)...

Expected behavior

Should build/run successfully

Actual behavior

Fails to compile with

 ERROR  Failed to compile with 1 error                                                                                                                         6:14:31 PM

 error  in ./node_modules/@feathersjs/hooks/esm/base.js

Module parse failed: Unexpected token (55:38)
You may need an appropriate loader to handle this file type, currently no loaders are configured to process this file. See https://webpack.js.org/concepts#loaders
|     }
|     middleware(middleware) {
>         this._middleware = middleware?.length ? middleware : null;
|         return this;
|     }

 @ ./node_modules/@feathersjs/hooks/esm/index.js 1:0-40 5:0-26 5:0-26 13:24-35 31:27-38
 @ ./node_modules/@feathersjs/feathers/lib/dependencies.js
 @ ./node_modules/@feathersjs/feathers/lib/index.js
 @ ./src/utils/feathers-api.js
 @ ./src/utils/index.js
 @ ./src/plugins/http.js
 @ ./src/main.js
 @ multi (webpack)-dev-server/client?http://<local-ip>:3000&sockPath=/sockjs-node (webpack)/hot/dev-server.js ./src/main.js

System configuration

Module versions:

packages.json

  "dependencies": {
    "@feathersjs/authentication-client": "^5.0.0-pre.17",
    "@feathersjs/feathers": "^5.0.0-pre.17",
    "@feathersjs/socketio-client": "^5.0.0-pre.17",
    "feathers-hooks-common": "^5.0.6",
    "socket.io-client": "^4.4.1",
    ...
  }

feathers.js file

import feathers from '@feathersjs/feathers'
import socketio from '@feathersjs/socketio-client'
import auth from '@feathersjs/authentication-client'
import io from 'socket.io-client'

const socket = io('http://localhost:3030', { transports: ['websocket'] })

// This variable name becomes the alias for this server.
export const api = feathers()
  .configure(socketio(socket))
  .configure(auth({ storage: window.localStorage }));

integrate it to existing feathers app

how to integrate this package to existing feathers app ?

Deno port chores

• Update module on https://Deno.land/x
• Automate publishing to npm
• Add this makefile for cross-platform development: https://github.com/MarkTiedemann/deno.mk
• Update README.md with the contribution details.

Unable to add a context collector for all methods of an object

Like:

const instance = new OtherDummy();

hooks(instance, {
  [ALL]: {
    context: withProps({ services }),
    middleware: [/* ... */]
  }
});

Wrong method name in context.method

Steps to reproduce

Please use this example. In this example, I called the create() method, but context.method gives me find instead of create.

Expected behavior

I expected to get the following output from the example:

context.method create

Actual behavior

I get the following output from the example:

context.method find

System configuration

Module versions:

@feathersjs/hooks: 0.5.0

NodeJS version: v12.18.3

Operating System: Linux

Error: Could not dynamically require "./base.js".

While trying to bundle with Vite, I encounter the following error:

[mf:err] Error: Could not dynamically require "./base.js". Please configure the dynamicRequireTargets or/and ignoreDynamicRequires option of @rollup/plugin-commonjs appropriately for this require call to work.
    at commonjsRequire (/Users/user/Sites/cf-feathers-2/dist/index.mjs:73:9)
    at /Users/user/Sites/cf-feathers-2/node_modules/@feathersjs/hooks/umd/index.js:22:23

This problem occurs even when just using @feathersjs/feathers, and not manually importing @feathersjs/hooks anywhere in user code.

I've tried fixing the issue using the dynamicRequireTargets and ignoreDynamicRequires options, and while they do fix this specific problem, they mess up the ability for modules in @feathersjs/feathers to properly import @feathersjs/hooks.

Allow to change the order of middleware

This is a follow-up for feathersjs/feathers#1590 to discuss options for allowing to change the order of middleware. Registering new middleware is currently possible using registerMiddleware:

const { hooks, registerMiddleware, getMiddleware } = require('@feathersjs/hooks');

// Hooks can be used with a function like this:
const sayHello = hooks(async name => {
  return `Hello ${name}!`;
}, [
  logRuntime,
  validateName
]);

registerMiddleware(sayHello, [
  otherHook
]);

However, it currently appends otherHook to the existing chain. One improvement possible would be to add a setMiddleware that allows to replace all middleware in combination with getMiddleware like this:

const { hooks, setMiddleware, getMiddleware } = require('@feathersjs/hooks');

// Hooks can be used with a function like this:
const sayHello = hooks(async name => {
  return `Hello ${name}!`;
}, [
  logRuntime,
  validateName
]);

setMiddleware(sayHello, [
  otherHook,
  ...getMiddleware(sayHello)
]);

Now otherHook would run first and your middleware chains are still explicitly defined.

Turn .params, .props and .defaults into hooks

While working on #35 I realized that most of what the hook manager does for .params, .props and .defaults could really be done by hooks themselves like this:

import { hooks, middleware, params, props, defaults } from '@feathersjs/hooks';

const sayHello = async (firstName, lastName) => {
  return `Hello ${firstName} ${lastName}!`;
};

const helloWithHooks = hooks(sayHello, [
  params('firstName', 'lastName'),
  props({ someProperty: true }),
  defaults((context) => ({
    lastName: 'unknown'
  })),
  async (context, next) => {
    // Do things here
    await next();
  }
]);

I did a quick benchmark and it seems to be just as fast as the current setup. I find it more consistent than having a separate API.

Brings webpack as dependency and many other due to that

Hi,

I just prepared a PR to our master that introduces @feathersjs/hooks. I was a little shocked by the many changes my package-lock.json showed. I have dozens of @webassemblyjs/... dependencies added:

I have the feeling that this is introduced by the webpack dependency that comes with this lib here:

I don't use webpack in my project - I use rollup so my question is: Does your lib need this? Can't this be a devDep?

Thanks,
Matt

Order of multiple hooks: mismatch of documentation and behavior

Steps to reproduce

Running the script from the documentation: https://github.com/feathersjs/hooks#middleware

Expected behavior

The output of the script should be (documented):

hook1 before
hook2 before
hook3 before
Hello David
hook3 after
hook2 after
hook1 after

Actual behavior

The output of the script is:

hook2 before
hook1 before
Hello David!
hook1 after
hook2 after
hook3 after

Thoughts

As hooks are decorators, it is clear that the last wrapped hook is run first.
So the question is, if the documentation should be adjusted, or if the order of the array should be reversed for wrapping. So that the first entry is wrapped as last one.

Brought up in Slack: https://feathersjs.slack.com/archives/C0HJE6A65/p1591009725097800?thread_ts=1590992656.097000&cid=C0HJE6A65