From beb5ae26177831622a2b8a4e815c6fbb88f7c0fd Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Tue, 21 May 2024 15:03:59 +0200 Subject: [PATCH] Remove the use of enterWith Signed-off-by: Matteo Collina --- README.md | 76 +++------------------------ example/{app.js => app.mjs} | 16 +++--- example/{do-work.js => do-work.mjs} | 6 +-- index.d.ts | 2 +- index.js | 46 +++++++--------- index.test-d.ts | 3 +- test/async-hooks.test.js | 81 ----------------------------- test/basic.test.js | 61 +++++++--------------- test/issue-7.test.mjs | 54 ------------------- 9 files changed, 56 insertions(+), 289 deletions(-) rename example/{app.js => app.mjs} (61%) rename example/{do-work.js => do-work.mjs} (51%) delete mode 100644 test/async-hooks.test.js delete mode 100644 test/issue-7.test.mjs diff --git a/README.md b/README.md index 23e7b48..a19bd09 100644 --- a/README.md +++ b/README.md @@ -12,18 +12,19 @@ npm i fastify fastify-asyncforge ## Usage ```js -// App.js +// app.mjs import fastify from 'fastify' -import { start } from 'fastify-asyncforge' import doWork from './do-work.mjs' +import asyncforge, { logger } from 'fastify-asyncforge' const app = fastify({ logger: true }) +await app.register(asyncforge) -// It's fundamental that `start` is called before any other plugins are registered, otherwise the helpers -// would not work as expected -await start(app) +app.runInAsyncScope(() => { + logger().info('hello') +}) app.decorate('foo', 'bar') app.decorateRequest('a') @@ -54,71 +55,6 @@ export default function doWork () { } ``` -### enterWith - -If you need to alter the current asynchronous context, you can use the `enterWith` helper. - -```js -import { before, describe, it } from "node:test"; -import Fastify from "fastify"; -import asyncforge, { app, start } from "fastify-asyncforge"; -import assert from "node:assert/strict"; - -let fastify; - -async function build(config) { - const server = await Fastify(); - - server.register(asyncforge) - server.decorate("config", config); - console.log("config from memo", app().config); - - return server; -} - -describe("support exiting from a context", () => { - before(async () => { - fastify = await build({ foo: "bar" }); - }); - - it("throws", () => { - assert.throws(app); - }); - - it("does not throw using enterWith", () => { - fastify.enterWith(); - assert.equal(app(), fastify); - }); -}); -``` - -## Usage together with other async_hooks tools (e.g. DataDog, OpenTelemetry, etc) - -If you are using `fastify-asyncforge` together with another `async_hooks` based tool, -you __must__ call `start` without adding an intermediate async function. -Alternatively, you'd need to use `.enterWith()` - -```js -import fastify from 'fastify' -import asyncforge from 'fastify-asyncforge' - -const fastify = Fastify() - -async function wrap () { - // This is necessary to make it throw - await 1 - await asyncforge.start(fastify) -} - -await wrap() - -// Calling .enterWith() is necessary or `asyncforge.app()` will throw -fastify.enterWith() -asyncforge.app() -``` - -If you are interested in knowing more, read https://github.com/nodejs/node/issues/53037. - ## License MIT diff --git a/example/app.js b/example/app.mjs similarity index 61% rename from example/app.js rename to example/app.mjs index 90232ef..5478401 100644 --- a/example/app.js +++ b/example/app.mjs @@ -1,17 +1,15 @@ -'use strict' - -const fastify = require('fastify') -const doWork = require('./do-work') -const { logger, start } = require('../index') +import fastify from 'fastify' +import doWork from './do-work.mjs' +import asyncforge, { logger } from '../index.js' const app = fastify({ logger: true }) -app.register(require('../index')) - -start(app) +await app.register(asyncforge) -logger().info('hello') +app.runInAsyncScope(() => { + logger().info('hello') +}) app.decorate('foo', 'bar') app.decorateRequest('a') diff --git a/example/do-work.js b/example/do-work.mjs similarity index 51% rename from example/do-work.js rename to example/do-work.mjs index fed2c1b..b57fc13 100644 --- a/example/do-work.js +++ b/example/do-work.mjs @@ -1,8 +1,6 @@ -'use strict' +import { logger, app, request, reply } from '../index.js' -const { logger, app, request, reply } = require('../') - -module.exports = function doWork () { +export default function doWork () { const log = logger() log.info({ foo: app().foo, diff --git a/index.d.ts b/index.d.ts index c425e8e..1d1a37d 100644 --- a/index.d.ts +++ b/index.d.ts @@ -8,7 +8,7 @@ import { declare module "fastify" { interface FastifyInstance { - enterWith: () => void; + runInAsyncScope (fn: () => T) : T; } } diff --git a/index.js b/index.js index 28dd03b..315583c 100644 --- a/index.js +++ b/index.js @@ -1,7 +1,7 @@ 'use strict' const fp = require('fastify-plugin') -const { memo, setAll } = require('asyncforge') +const { memo, create } = require('asyncforge') const app = memo('fastify.app') const request = memo('fastify.request') @@ -9,39 +9,33 @@ const reply = memo('fastify.reply') const logger = memo('fastify.logger') const fastifyAsyncForge = fp(function (fastify, opts, next) { - fastify.decorate('enterWith', function () { - try { - app() - } catch { - setAll({ - [app.key]: this, - [logger.key]: this.log - }) - } + const store = create() + + fastify.decorate('runInAsyncScope', function (fn) { + return store.run(fn) }) - fastify.addHook('onRequest', async function (req, res) { - setAll({ - [app.key]: this, - [request.key]: req, - [reply.key]: res, - [logger.key]: req.log + fastify.addHook('onRequest', function (req, res, next) { + const store = create() + + // We use callbacks because we cannot propagate through async/await + store.run(() => { + app.set(this) + request.set(req) + reply.set(res) + logger.set(req.log) + next() }) }) - next() -}) -function start (fastify) { - setAll({ - [app.key]: fastify, - [logger.key]: fastify.log + store.run(() => { + app.set(fastify) + logger.set(fastify.log) + next() }) - - return fastify.register(fastifyAsyncForge) -} +}) module.exports = fastifyAsyncForge -module.exports.start = start module.exports.app = app module.exports.request = request module.exports.reply = reply diff --git a/index.test-d.ts b/index.test-d.ts index 0608401..ac21819 100644 --- a/index.test-d.ts +++ b/index.test-d.ts @@ -13,12 +13,11 @@ import fastify, { const fastifyInstance = fastify(); expectAssignable(fastifyInstance.register(fastifyasyncforge)); expectAssignable(fastifyasyncforge); -expectType(fastifyInstance.enterWith()); +expectType(fastifyInstance.runInAsyncScope(() => 42)); // app expectAssignable(fastifyInstance); expectAssignable(app()); -expectType(app().enterWith()); expectAssignable(app>()); expectError(app()); expectError({}); diff --git a/test/async-hooks.test.js b/test/async-hooks.test.js deleted file mode 100644 index 7084571..0000000 --- a/test/async-hooks.test.js +++ /dev/null @@ -1,81 +0,0 @@ -'use strict' - -const { test } = require('node:test') -const { createHook } = require('node:async_hooks') -const Fastify = require('fastify') -const tspl = require('@matteo.collina/tspl') -const fastifyAsyncForge = require('../') -const { app, request, reply, logger } = fastifyAsyncForge - -// Adding an async_hooks hook to make sure we catch the issue -const hook = createHook({ init () {} }) -hook.enable() - -test('async_hooks lose context', async (t) => { - const p = tspl(t, { plan: 8 }) - const fastify = Fastify() - t.after(() => fastify.close()) - - async function wrap () { - // This is necessary to make it throw - await 1 - await fastifyAsyncForge.start(fastify) - } - - await wrap() - - // This is expected to throw due to https://github.com/nodejs/node/issues/53037 - p.throws(logger) - p.throws(app) - - fastify.get('/', async function (_request, _reply) { - p.strictEqual(app(), this) - p.strictEqual(request(), _request) - p.strictEqual(reply(), _reply) - p.strictEqual(logger(), _request.log) - return { hello: 'world' } - }) - - await fastify.listen({ port: 0 }) - - // We use fetch() here to make sure we are breaking the async context - const res = await fetch(fastify.listeningOrigin) - p.strictEqual(res.status, 200) - p.deepStrictEqual(await res.json(), { - hello: 'world' - }) -}) - -test('enterWith fixes it', async (t) => { - const p = tspl(t, { plan: 8 }) - const fastify = Fastify() - t.after(() => fastify.close()) - - async function wrap () { - await 1 - await fastifyAsyncForge.start(fastify) - } - - await wrap() - await fastify.enterWith() - - p.strictEqual(logger(), fastify.log) - p.strictEqual(app(), fastify) - - fastify.get('/', async function (_request, _reply) { - p.strictEqual(app(), this) - p.strictEqual(request(), _request) - p.strictEqual(reply(), _reply) - p.strictEqual(logger(), _request.log) - return { hello: 'world' } - }) - - await fastify.listen({ port: 0 }) - - // We use fetch() here to make sure we are breaking the async context - const res = await fetch(fastify.listeningOrigin) - p.strictEqual(res.status, 200) - p.deepStrictEqual(await res.json(), { - hello: 'world' - }) -}) diff --git a/test/basic.test.js b/test/basic.test.js index ea77df1..43ccec4 100644 --- a/test/basic.test.js +++ b/test/basic.test.js @@ -11,10 +11,12 @@ test('basic helpers with start', async (t) => { const p = tspl(t, { plan: 7 }) const fastify = Fastify() - await fastifyAsyncForge.start(fastify) + await fastify.register(fastifyAsyncForge) - p.strictEqual(logger(), fastify.log) - p.strictEqual(app(), fastify) + fastify.runInAsyncScope(() => { + p.strictEqual(logger(), fastify.log) + p.strictEqual(app(), fastify) + }) fastify.get('/', async function (_request, _reply) { p.strictEqual(app(), this) @@ -61,43 +63,16 @@ test('basic helpers without start', async (t) => { await p.completed }) -test('fastify.enterWith', async (t) => { - const p = tspl(t, { plan: 7 }) - const fastify = Fastify() - - await fastify.register(fastifyAsyncForge) - - fastify.enterWith() - - p.strictEqual(logger(), fastify.log) - p.strictEqual(app(), fastify) - - fastify.get('/', async function (_request, _reply) { - p.strictEqual(app(), this) - p.strictEqual(request(), _request) - p.strictEqual(reply(), _reply) - p.strictEqual(logger(), _request.log) - return { hello: 'world' } - }) - - const res = await fastify.inject({ - method: 'GET', - url: '/' - }) - - p.strictEqual(res.statusCode, 200) - - await p.completed -}) - test('with additional onRequest hook', async (t) => { const p = tspl(t, { plan: 15 }) const fastify = Fastify() - await fastifyAsyncForge.start(fastify) + await fastify.register(fastifyAsyncForge) - p.strictEqual(logger(), fastify.log) - p.strictEqual(app(), fastify) + fastify.runInAsyncScope(() => { + p.strictEqual(logger(), fastify.log) + p.strictEqual(app(), fastify) + }) fastify.addHook('onRequest', async function b (_request, _reply) { p.strictEqual(app(), this) @@ -136,16 +111,18 @@ test('onRequest hook added before registration fails', async (t) => { const fastify = Fastify() fastify.addHook('onRequest', async function b (_request, _reply) { - p.strictEqual(app(), this) - p.strictEqual(logger(), fastify.log) - p.strictEqual(request(), undefined) - p.strictEqual(reply(), undefined) + p.throws(app) + p.throws(logger) + p.throws(request) + p.throws(reply) }) - await fastifyAsyncForge.start(fastify) + await fastify.register(fastifyAsyncForge) - p.strictEqual(logger(), fastify.log) - p.strictEqual(app(), fastify) + fastify.runInAsyncScope(() => { + p.strictEqual(logger(), fastify.log) + p.strictEqual(app(), fastify) + }) fastify.get('/', { onRequest: async function a (_request, _reply) { diff --git a/test/issue-7.test.mjs b/test/issue-7.test.mjs deleted file mode 100644 index cc9806e..0000000 --- a/test/issue-7.test.mjs +++ /dev/null @@ -1,54 +0,0 @@ -import { before, describe, it } from 'node:test' -import Fastify from 'fastify' -import asyncforge, { app, start } from '../index.js' -import assert from 'node:assert/strict' - -let fastify - -async function build (config) { - const server = await Fastify() - - server.decorate('config', config) - await start(server) - - return server -} - -async function buildWithRegister (config) { - const server = await Fastify() - - server.register(asyncforge) - server.decorate('config', config) - - return server -} - -describe('support exiting from a context / start', () => { - before(async () => { - fastify = await build({ foo: 'bar' }) - }) - - it('throws', () => { - assert.throws(app) - }) - - it('does not throw using enterWith', () => { - fastify.enterWith() - assert.equal(app(), fastify) - }) -}) - -describe('support exiting from a context / register', () => { - before(async () => { - fastify = await buildWithRegister({ foo: 'bar' }) - }) - - it('throws', () => { - assert.throws(app) - }) - - it('does not throw using enterWith', () => { - fastify.enterWith() - assert.equal(app(), fastify) - }) -})