Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert hooks system to a middleware system #2071

Merged
merged 43 commits into from
May 21, 2024
Merged

Convert hooks system to a middleware system #2071

merged 43 commits into from
May 21, 2024

Conversation

benjie
Copy link
Member

@benjie benjie commented May 21, 2024

Description

In an earlier version of v5 I introduced the hooks system (e.g. plugin.grafserv.hooks.ruruHTMLParts). The intention of this system was to give people a chance to quickly customize certain things, e.g. tweaking the configuration object for Ruru.

But it turns out that sometimes we want to wrap a code block in hooks. At first I attempted to do this by having hooks return a callback function to be called once the block completes, this was inspired by the pattern used by the Envelop plugin system. However, the issue with this is that it doesn't integrate well with things like Node's async_hooks system unless the wrapped code is synchronous. To solve this, I've switched to more of a middleware system inspired by Koa. This allows you to do things like timing execution and reporting on the number of errors:

const TimingPlugin: GraphileConfig.Plugin = {
  name: "TimingPlugin",
  version: "0.0.0",

  grafast: {
    middlewares: {
      execute(next, event) {
        const start = process.hrtime.bigint();
        return next.callback((error, result) => {
          const stop = process.hrtime.bigint();
          const durationInNanoseconds = stop - start;
          const milliseconds = Number(durationInNanoseconds) / 1e6;
          if (error !== undefined) {
            console.log(`Execution failed and took ${milliseconds}ms`);
            throw error;
          } else {
            if ("errors" in result && result.errors != null) {
              console.log(
                `Execution took ${milliseconds}ms (${result.errors.length} errors)`,
              );
            } else {
              console.log(`Execution took ${milliseconds}ms (no errors)`);
            }
            return result;
          }
        });
      },
    },
  },
};

And also hooking into async_hooks to track where errors/promises are generated, etc.

It's important to note that some of the middlewares are synchronous and some asynchronous (and that we try not to create promises unless they're necessary) so although it's possible to write many middlewares as async/await functions, we recommend against it in plugin code because it forces promises, causing performance issues and also preventing synchronous paths which may be used, for example executeSync(). This is why we have the next.callback() method above rather than calling next() directly.

Middleware are experimental, and could change in minor revisions.

In many cases, middleware replace hooks (since every hook can theoretically be implemented via middleware).

Performance impact

Marginal if middlewares are not in use.

Security impact

Increases surface area, and middleware can be written in a way that compromises the system (but that's the responsibility of the person writing the middleware).

Checklist

  • My code matches the project's code style and yarn lint:fix passes.
  • I've added tests for the new feature, and yarn test passes.
  • I have detailed the new feature in the relevant documentation.
  • I have added this feature to 'Pending' in the RELEASE_NOTES.md file (if one exists).
  • If this is a breaking change I've explained why.

benjie added 30 commits May 17, 2024 11:57
Copy link

changeset-bot bot commented May 21, 2024

🦋 Changeset detected

Latest commit: 16aff7f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 17 packages
Name Type
graphile-build Patch
postgraphile Patch
graphile-config Patch
@dataplan/pg Patch
grafserv Patch
grafast Patch
graphile-build-pg Patch
graphile-utils Patch
pgl Patch
graphile Patch
@localrepo/grafast-bench Patch
@grafserv/persisted Patch
ruru Patch
@dataplan/json Patch
ruru-components Patch
@localrepo/grafast-website Patch
graphile-export Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

benjie added 2 commits May 21, 2024 11:59
…and `requestContext`

directly; passing these through additional arguments is now deprecated.

`plugin.grafast.hooks.args` is now `plugin.grafast.middleware.prepareArgs`, and
the signature has changed - you must be sure to call the `next()` function and
ctx/resolvedPreset can be extracted directly from `args`:

```diff
 const plugin = {
   grafast: {
-    hooks: {
+    middleware: {
-      args({ args, ctx, resolvedPreset }) {
+      prepareArgs(next, { args }) {
+        const { requestContext: ctx, resolvedPreset } = args;
         // ...
+        return next();
       }
     }
   }
 }
```
@benjie benjie marked this pull request as ready for review May 21, 2024 12:49
@benjie benjie merged commit c734994 into main May 21, 2024
36 checks passed
@benjie benjie deleted the hooks branch May 21, 2024 15:22
@benjie benjie mentioned this pull request May 23, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

1 participant