diff --git a/CHANGELOG.md b/CHANGELOG.md index c505f0f7..65bb433a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,11 @@ The format follows semantic versioning principles. ### Fixed +- `DotPath` (the key type behind `config()` and `configure()`) is now depth-bounded and stops at built-in object types, so configs holding deep or self-referencing library types (nodemailer transports) no longer fail to typecheck with "circularly references itself". The `@ts-expect-error` workarounds in the H3 template and the notifications config are gone. +- `build:templates` builds the templates one at a time; run in parallel, each template's prebuild rebuilt the shared packages concurrently and the builds clobbered each other. +- Fixed type errors in `@arkstack/auth` (`AuthenticationException` request typing and two tests) and in `@arkstack/common`'s `renderError`, which used the `view()` global without a type for it. +- Express `after` middleware no longer resumes the router once the response has finished. It used to receive the real `next`, so a middleware such as `requestLogger()` sent each request on through the remaining routes after responding, raising "Cannot set headers after they are sent to the client". +- `UserNotificationCenter.markAllRead()` now marks every unread notification read; it used a single-row `update()` and marked only one. `unreadForUser(user, perPage)` now honours `perPage`, and both filter unread rows with `whereNull('readAt')`. - `UserNotificationCenter.send()` (and `Notification.db().broadcast()`) now broadcast on the user's channel. They previously sent to the user's push tokens as channels, so nothing was broadcast at all for users without tokens. Apps on the `firebase` transport still reach their users' devices without changes, because device push is on there unless `drivers.realtime.push` is `false`. - `Notification.db().broadcast()` now returns the builder, so it chains as documented. - The Pusher auth route registered by `registerAuthRoute()` now answers with HTTP 200 (pusher-js rejects any other status), registers on the H3 driver instead of the Express one, and authorizes through the channel registry. diff --git a/package.json b/package.json index 3c25ae68..9b16b4f4 100644 --- a/package.json +++ b/package.json @@ -13,7 +13,7 @@ "test:watch": "vitest", "test:coverage": "vitest run --coverage", "test:integration": "vitest run tests/integration", - "build:templates": "pnpm -r --filter \"*-app\" build", + "build:templates": "pnpm -r --workspace-concurrency=1 --filter \"*-app\" build", "build:packages": "pnpm -r --filter \"@arkstack/*\" build", "prepare:release-version": "node scripts/prepare-release-version.mjs", "publish:packages": "pnpm -r --filter \"@arkstack/*\" --filter create-arkstack publish --access public", diff --git a/packages/auth/src/Exceptions/AuthenticationException.ts b/packages/auth/src/Exceptions/AuthenticationException.ts index f67d2a45..3a57c4b4 100644 --- a/packages/auth/src/Exceptions/AuthenticationException.ts +++ b/packages/auth/src/Exceptions/AuthenticationException.ts @@ -1,10 +1,11 @@ import { Request, Response, type RequestSource, type ResponseSource } from '@arkstack/http' import { Exception } from '@arkstack/common' +import type { User } from '@app/models/User' export class AuthenticationException extends Exception { #errors?: Record - #request?: Request + #request?: Request #response?: Response statusCode: number = 401 name: string @@ -12,7 +13,7 @@ export class AuthenticationException extends Exception { constructor( message: string = 'Authentication failed', ctx?: { - req?: Request | RequestSource, + req?: Request | RequestSource, res?: Response | ResponseSource, status?: number, errors?: Record @@ -31,7 +32,7 @@ export class AuthenticationException extends Exception { void this.#request } - errors (): Record | undefined { + errors(): Record | undefined { return this.#errors } } diff --git a/packages/auth/tests/express.test.ts b/packages/auth/tests/express.test.ts index f493a817..093ec5c4 100644 --- a/packages/auth/tests/express.test.ts +++ b/packages/auth/tests/express.test.ts @@ -79,12 +79,12 @@ describe('Express auth integration', () => { return { authToken: req?.authToken, authUserFromAuthId: (req?.auth as Auth | undefined)?.user()?.id, - authUserId: req?.authUser?.id, + authUserId: (req?.authUser as { id?: unknown } | undefined)?.id, constructorName: req?.constructor.name, hasIp: Boolean(req?.ip), hasSource: Boolean(req?.source), isArkstackRequest: req instanceof ArkstackRequest, - userId: req?.user?.id, + userId: (req?.user as { id?: unknown } | undefined)?.id, } }, [auth]) Router.apply(router) diff --git a/packages/auth/tests/two-factor.test.ts b/packages/auth/tests/two-factor.test.ts index 03db7c00..dfdda63d 100644 --- a/packages/auth/tests/two-factor.test.ts +++ b/packages/auth/tests/two-factor.test.ts @@ -8,7 +8,7 @@ const user = { email: 'two-factor@example.com', name: 'Two Factor', password: 'password', -} as User +} as unknown as User describe('TwoFactor', () => { it('normalizes supported methods', () => { diff --git a/packages/common/package.json b/packages/common/package.json index 017daf5d..40111fbf 100644 --- a/packages/common/package.json +++ b/packages/common/package.json @@ -64,5 +64,10 @@ "arkormx": { "optional": true } + }, + "inlinedDependencies": { + "clear-router": "2.9.3", + "dayjs": "1.11.20", + "kanun": "1.2.0" } } diff --git a/packages/common/src/network.ts b/packages/common/src/network.ts index 5dac002f..bf27d456 100644 --- a/packages/common/src/network.ts +++ b/packages/common/src/network.ts @@ -45,6 +45,11 @@ export const bootWithDetectedPort = async ) => { renderSync(): string } +} + export const renderError = ({ message = 'An unexpected error occurred.', stack, @@ -69,7 +74,7 @@ export const renderError = ({ title = titleMap[code] || title || 'Error' - return globalThis.view('~arkstack/common.error', { + return (globalThis as unknown as ViewGlobal).view('~arkstack/common.error', { code, title, stack, diff --git a/packages/common/src/types.ts b/packages/common/src/types.ts index 39dd3296..1998cc6d 100644 --- a/packages/common/src/types.ts +++ b/packages/common/src/types.ts @@ -140,16 +140,29 @@ export type DotPathValue = ? T[P] : never -export type DotPath = T extends Primitive +/** Remaining depth for {@link DotPath}: index by the current depth to get the next. */ +type DotPathDepth = [never, 0, 1, 2, 3, 4, 5, 6] + +/** Values {@link DotPath} treats as leaves instead of walking into. */ +type DotPathLeaf = Primitive | Date | RegExp | Promise | Map | Set | Uint8Array + +/** + * Dot-separated key paths of `T`, e.g. `'mail.from.address'`. Bounded in depth + * so deep or self-referencing library types (mail transports, streams) stay + * finite instead of being reported as circular. + */ +export type DotPath = [Depth] extends [never] + ? never + : T extends DotPathLeaf ? never : T extends any[] ? never : { - [K in keyof T & string]: T[K] extends Primitive + [K in keyof T & string]: T[K] extends DotPathLeaf ? `${K}` : T[K] extends any[] ? `${K}` - : `${K}` | `${K}.${DotPath}` + : `${K}` | `${K}.${DotPath}` }[keyof T & string] /** diff --git a/packages/driver-express/src/ExpressDriver.ts b/packages/driver-express/src/ExpressDriver.ts index ef8700f8..5ff90da1 100644 --- a/packages/driver-express/src/ExpressDriver.ts +++ b/packages/driver-express/src/ExpressDriver.ts @@ -120,9 +120,12 @@ export class ExpressDriver extends ArkstackKitDriver { const entry = resolveMiddleware(instance) if (pos === 'after') { - app.use(async (req, res, next) => { - res.once('finish', async () => { - await entry(req, res, next) + app.use((req, res, next) => { + // The response is already sent, so `next` must not resume the router. + res.once('finish', () => { + Promise.resolve(entry(req, res, () => { })).catch((error) => { + Logger.error(error instanceof Error ? error.message : String(error)) + }) }) next() }) diff --git a/packages/driver-express/tests/after-middleware.test.ts b/packages/driver-express/tests/after-middleware.test.ts new file mode 100644 index 00000000..5eab089c --- /dev/null +++ b/packages/driver-express/tests/after-middleware.test.ts @@ -0,0 +1,33 @@ +import { describe, expect, it, vi } from 'vitest' +import express from 'express' +import request from 'parasito' + +import { ExpressDriver } from '../src/index' + +describe('ExpressDriver after middleware', () => { + it('runs once the response finishes without re-entering the router', async () => { + const app = express() + const handler = vi.fn((_req: express.Request, res: express.Response) => { + res.json({ ok: true }) + }) + const fallthrough = vi.fn((_req: express.Request, _res: express.Response, next: express.NextFunction) => next()) + let finished!: () => void + const done = new Promise((resolve) => (finished = resolve)) + const after = vi.fn((_req: unknown, _res: unknown, next: () => void) => { + next() + finished() + }) + + new ExpressDriver({ bindRouter: vi.fn() }).applyMiddleware(app, { after: [after] } as never) + app.get('/thing', handler) + app.use(fallthrough) + + const res = await request(app).get('/thing') + await done + + expect(res.status).toBe(200) + expect(after).toHaveBeenCalledOnce() + expect(handler).toHaveBeenCalledOnce() + expect(fallthrough).not.toHaveBeenCalled() + }) +}) diff --git a/packages/driver-h3/tests/after-middleware.test.ts b/packages/driver-h3/tests/after-middleware.test.ts new file mode 100644 index 00000000..4f086031 --- /dev/null +++ b/packages/driver-h3/tests/after-middleware.test.ts @@ -0,0 +1,22 @@ +import { describe, expect, it, vi } from 'vitest' +import { H3 } from 'h3' + +import { H3Driver } from '../src/index' + +describe('H3Driver after middleware', () => { + it('runs after the handler, which runs once, and keeps its response', async () => { + const app = new H3() + const handler = vi.fn(() => ({ ok: true })) + const after = vi.fn((_event: unknown, next: () => unknown) => next()) + + new H3Driver({ bindRouter: vi.fn() } as never).applyMiddleware(app, { after: [after] } as never) + app.get('/thing', handler) + + const res = await app.fetch(new Request('http://localhost/thing')) + + expect(res.status).toBe(200) + expect(await res.json()).toEqual({ ok: true }) + expect(handler).toHaveBeenCalledOnce() + expect(after).toHaveBeenCalledOnce() + }) +}) diff --git a/packages/http/src/Request.ts b/packages/http/src/Request.ts index bb299877..bcea4e3a 100644 --- a/packages/http/src/Request.ts +++ b/packages/http/src/Request.ts @@ -48,35 +48,35 @@ export class Request extends BaseRequest { private currentAuthUser?: TUser private currentAuthToken?: string - get user (): TUser | undefined { + get user(): TUser | undefined { return this.getSourceRequest()?.user ?? this.currentUser } - set user (user: TUser | undefined) { + set user(user: TUser | undefined) { this.currentUser = user } - get auth (): unknown { + get auth(): unknown { return this.getSourceRequest()?.auth ?? this.currentAuth } - set auth (auth: unknown) { + set auth(auth: unknown) { this.currentAuth = auth } - get authUser (): TUser | undefined { + get authUser(): TUser | undefined { return this.getSourceRequest()?.authUser ?? this.currentAuthUser } - set authUser (user: TUser | undefined) { + set authUser(user: TUser | undefined) { this.currentAuthUser = user } - get authToken (): string | undefined { + get authToken(): string | undefined { return this.getSourceRequest()?.authToken ?? this.currentAuthToken } - set authToken (token: string | undefined) { + set authToken(token: string | undefined) { this.currentAuthToken = token } @@ -105,7 +105,7 @@ export class Request extends BaseRequest { setFallbackRequest(this) } - static from ( + static from( source?: Request | RequestSource ): Request | undefined { if (!source) { @@ -139,11 +139,11 @@ export class Request extends BaseRequest { }) } - header (name: string): string { + header(name: string): string { return this.headers[name.toLowerCase()] } - bearerToken (): string | null { + bearerToken(): string | null { const authorization = this.header('authorization') if (!authorization?.startsWith('Bearer ')) { @@ -153,7 +153,7 @@ export class Request extends BaseRequest { return authorization.substring(7) } - setUser (user: TUser) { + setUser(user: TUser) { this.user = user if (isRecord(this.source)) { @@ -163,7 +163,7 @@ export class Request extends BaseRequest { return this } - setAuthentication (auth: TAuth, user: TUser, token?: string) { + setAuthentication(auth: TAuth, user: TUser, token?: string) { this.auth = auth this.authUser = user this.authToken = token @@ -178,7 +178,7 @@ export class Request extends BaseRequest { return this } - syncFromSource () { + syncFromSource() { if (!isRecord(this.source)) { return this } @@ -193,13 +193,13 @@ export class Request extends BaseRequest { return this } - private getSourceRequest (): RequestSource | undefined { + private getSourceRequest(): RequestSource | undefined { return isRecord(this.source) ? this.source as RequestSource : undefined } - clearAuthentication () { + clearAuthentication() { this.auth = undefined this.authUser = undefined this.authToken = undefined diff --git a/packages/notifications/src/UserNotificationCenter.ts b/packages/notifications/src/UserNotificationCenter.ts index a29910f1..8ea71eff 100644 --- a/packages/notifications/src/UserNotificationCenter.ts +++ b/packages/notifications/src/UserNotificationCenter.ts @@ -78,10 +78,10 @@ export class UserNotificationCenter { static async unreadForUser(user: User, perPage?: number): Promise { const Model = await this.getModel() - const query = Model.query().where({ userId: user.id, readAt: null }) + const query = Model.query().where({ userId: user.id }).whereNull('readAt') if (perPage) { - return await query.paginate() + return await query.paginate(perPage) } return await query.get() @@ -112,13 +112,11 @@ export class UserNotificationCenter { static async markAllRead(user: User, ids?: string[] | number[]) { const Model = await this.getModel() - const query = Model.query().where({ userId: user.id, readAt: null }) + const query = Model.query().where({ userId: user.id }).whereNull('readAt') + const scoped = ids ? query.whereIn('id', ids) : query - if (ids) { - query.whereIn('id', ids) - } - - await query.update({ readAt: new Date() }) + // `update` changes a single row; `updateFrom` updates every match. + await scoped.updateFrom({ readAt: new Date() }) } /** diff --git a/packages/notifications/src/config.ts b/packages/notifications/src/config.ts index 452fc15c..62661015 100644 --- a/packages/notifications/src/config.ts +++ b/packages/notifications/src/config.ts @@ -2,7 +2,6 @@ import { DotPath, DotPathValue, config } from '@arkstack/common' import { NotificationConfig } from './types' -// @ts-expect-error Just ignore the old fool export const configure = >( key: T, defaultValue?: unknown, diff --git a/packages/notifications/tests/broadcast.test.ts b/packages/notifications/tests/broadcast.test.ts index 4a3587bb..b564f1b3 100644 --- a/packages/notifications/tests/broadcast.test.ts +++ b/packages/notifications/tests/broadcast.test.ts @@ -371,3 +371,58 @@ describe('FirebaseDatabaseRealtimeDriver', () => { })) }) }) + +describe('UserNotificationCenter queries', () => { + /** A fake model whose query records each builder call. */ + const fakeModel = () => { + const calls: Array<[string, unknown[]]> = [] + const query: Record unknown> = {} + + for (const method of ['where', 'whereNull', 'whereIn']) { + query[method] = (...args) => { + calls.push([method, args]) + + return query + } + } + + for (const method of ['updateFrom', 'update', 'paginate', 'get']) { + query[method] = async (...args) => { + calls.push([method, args]) + + return method === 'updateFrom' ? 3 : [] + } + } + + vi.spyOn(UserNotificationCenter as any, 'getModel').mockResolvedValue({ query: () => query }) + + return calls + } + + it('marks every unread notification read, not just one', async () => { + const calls = fakeModel() + + await UserNotificationCenter.markAllRead({ id: 7 } as never) + + expect(calls.map(([method]) => method)).toEqual(['where', 'whereNull', 'updateFrom']) + expect(calls[1][1]).toEqual(['readAt']) + expect(calls.some(([method]) => method === 'update')).toBe(false) + }) + + it('limits marking read to the given ids', async () => { + const calls = fakeModel() + + await UserNotificationCenter.markAllRead({ id: 7 } as never, ['a', 'b']) + + expect(calls).toContainEqual(['whereIn', ['id', ['a', 'b']]]) + }) + + it('paginates unread notifications with the requested page size', async () => { + const calls = fakeModel() + + await UserNotificationCenter.unreadForUser({ id: 7 } as never, 25) + + expect(calls).toContainEqual(['paginate', [25]]) + expect(calls).toContainEqual(['whereNull', ['readAt']]) + }) +}) diff --git a/packages/notifications/tests/notification.test.ts b/packages/notifications/tests/notification.test.ts index 4d674161..6ab7c0e5 100644 --- a/packages/notifications/tests/notification.test.ts +++ b/packages/notifications/tests/notification.test.ts @@ -36,10 +36,16 @@ const mocks = vi.hoisted(() => { const deleteQuery = vi.fn(async () => undefined) const get = vi.fn(async () => []) const update = vi.fn(async () => undefined) + const updateFrom = vi.fn(async () => 0) + const whereNull = vi.fn(() => ({ + get, + updateFrom, + })) const where = vi.fn(() => ({ delete: deleteQuery, get, update, + whereNull, })) const africasTalkingSend = vi.fn(async payload => ({ provider: 'africastalking', @@ -73,6 +79,8 @@ const mocks = vi.hoisted(() => { sendMail, createTransport, twilioCreate, + updateFrom, + whereNull, deleteQuery, get, update, @@ -540,7 +548,8 @@ describe('Notification', () => { expect(mocks.get).toHaveBeenCalledTimes(1) await expect(UserNotificationCenter.unreadForUser(user)).resolves.toEqual([]) - expect(mocks.where).toHaveBeenLastCalledWith({ userId: 12, readAt: null }) + expect(mocks.where).toHaveBeenLastCalledWith({ userId: 12 }) + expect(mocks.whereNull).toHaveBeenLastCalledWith('readAt') expect(mocks.get).toHaveBeenCalledTimes(2) await UserNotificationCenter.markRead(notification) @@ -552,8 +561,8 @@ describe('Notification', () => { expect(mocks.where).toHaveBeenLastCalledWith({ id: 100 }) await UserNotificationCenter.markAllRead(user) - expect(mocks.where).toHaveBeenLastCalledWith({ userId: 12, readAt: null }) - expect(mocks.update).toHaveBeenLastCalledWith({ readAt: expect.any(Date) }) + expect(mocks.where).toHaveBeenLastCalledWith({ userId: 12 }) + expect(mocks.updateFrom).toHaveBeenLastCalledWith({ readAt: expect.any(Date) }) await UserNotificationCenter.delete(notification) expect(mocks.where).toHaveBeenLastCalledWith({ id: 99 }) diff --git a/templates/h3/src/core/app.ts b/templates/h3/src/core/app.ts index 61cfa681..12bce8f2 100644 --- a/templates/h3/src/core/app.ts +++ b/templates/h3/src/core/app.ts @@ -50,7 +50,6 @@ export default class Application extends Arkstack { await this.driver.mountPublicAssets(this.app, 'public') // Apply all middleware - // @ts-expect-error config cannot be discovered outside app scaffolds await this.driver.applyMiddleware(this.app, config('middleware') as never) // Bind the router