Changed limit-service to TypeScript and moved Ghost's queries out of it

.markdownlint-cli2.jsonc

@@ -1,8 +1,4 @@
 {
-  // The imported limit-service documentation is kept exactly as it arrived from
-  // TryGhost/SDK so it can be diffed against the source. It is linted once it is
-  // rewritten alongside the package.
-  "ignores": ["packages/limit-service/**"],
   "config": {
     "default": false,
     "MD011": true,

.oxfmtrc.json

@@ -18,7 +18,6 @@
     "koenig/kg-simplemde/debug/**",
     "koenig/koenig-lexical/**",
     "packages/i18n/locales/**",
-    "packages/limit-service/**",
     ".changeset/ledger.yaml"
   ]
 }

apps/admin-x-framework/src/hooks/use-limiter.ts

@@ -6,34 +6,17 @@ import { useBrowseNewsletters } from '../api/newsletters';
 import { useBrowseRoles } from '../api/roles';
 import { useBrowseUsers } from '../api/users';
 import { HostLimitError } from '../utils/errors';
+import type { Counter, GhostErrorOptions, LimitConfig } from '@tryghost/limit-service';
 
 const limitServiceImport = import('@tryghost/limit-service');
 
 // limit-service constructs its misconfiguration error with a single options object
 class IncorrectUsageError extends Error {
-  constructor({ message }: { message: string }) {
+  constructor({ message }: GhostErrorOptions) {
     super(message);
   }
 }
 
-interface LimiterLimits {
-  staff?: {
-    max?: number;
-    error?: string;
-    currentCountQuery?: () => Promise<number>;
-  };
-  members?: {
-    max?: number;
-    error?: string;
-    currentCountQuery?: () => Promise<number>;
-  };
-  newsletters?: {
-    max?: number;
-    error?: string;
-    currentCountQuery?: () => Promise<number>;
-  };
-}
-
 export interface Limiter {
   isLimited: (limitName: string) => boolean;
   isDisabled: (limitName: string) => boolean;
@@ -89,11 +72,12 @@ export const useLimiter = (): Limiter => {
       return noOpLimiter;
     }
 
-    const limits = { ...config.hostSettings.limits } as LimiterLimits;
     const limiter = new LimitService();
 
-    if (limits.staff) {
-      limits.staff.currentCountQuery = () => {
+    // How Admin counts, as opposed to how the server does. The limit service asks for a
+    // number and neither side has to know how the other arrives at one.
+    const counters: Record<string, Counter> = {
+      staff: () => {
         // Keep the existing first-page behavior for this move. Full pagination is tracked in
         // PLA-369 because excluded users/invites can push countable staff onto later pages.
         const staffUsers = users.filter(
@@ -105,26 +89,23 @@ export const useLimiter = (): Limiter => {
           return role?.name !== 'Contributor';
         });
 
-        return Promise.resolve(staffUsers.length + staffInvites.length);
-      };
-    }
+        return staffUsers.length + staffInvites.length;
+      },
 
-    if (limits.members) {
-      limits.members.currentCountQuery = async () => {
+      members: async () => {
         const { data: members } = await fetchMembers();
         return members?.meta?.pagination?.total || 0;
-      };
-    }
+      },
 
-    if (limits.newsletters) {
-      limits.newsletters.currentCountQuery = async () => {
+      newsletters: async () => {
         const { data: { pages } = { pages: [] } } = await fetchNewsletters();
         return pages[0].meta?.pagination.total || 0;
-      };
-    }
+      },
+    };
 
     limiter.loadLimits({
-      limits,
+      limits: config.hostSettings.limits as Record<string, LimitConfig>,
+      counters,
       helpLink,
       errors: {
         HostLimitError,
@@ -134,9 +115,12 @@ export const useLimiter = (): Limiter => {
 
     return {
       isLimited: (limitName: string): boolean => limiter.isLimited(limitName),
-      isDisabled: (limitName: string): boolean => limiter.isDisabled(limitName),
-      checkWouldGoOverLimit: (limitName: string): Promise<boolean> =>
-        limiter.checkWouldGoOverLimit(limitName),
+      // Both answer `undefined` for a limit this site does not have, which every caller
+      // already reads as falsy. Said explicitly now the package ships its own types; the
+      // hand-written declarations this replaces claimed a plain boolean.
+      isDisabled: (limitName: string): boolean => limiter.isDisabled(limitName) ?? false,
+      checkWouldGoOverLimit: async (limitName: string): Promise<boolean> =>
+        (await limiter.checkWouldGoOverLimit(limitName)) ?? false,
       errorIfWouldGoOverLimit: (
         limitName: string,
         metadata: Record<string, unknown> = {},

apps/admin/vite.config.ts

@@ -49,11 +49,7 @@ export default defineConfig(({ command }) => ({
     // forwardConsole: { logLevels: ['warn', 'error'] }
   },
   optimizeDeps: {
-    // limit-service is CommonJS, and Vite only converts CommonJS while pre-bundling. A
-    // workspace package is treated as source and served raw, where `module` does not exist,
-    // so the import fails and the limiter silently falls back to reporting every host limit
-    // as absent. Force it through the pre-bundler until the package itself is converted.
-    include: ['@tryghost/koenig-lexical', '@tryghost/limit-service'],
+    include: ['@tryghost/koenig-lexical'],
   },
   resolve: sharedResolve,
   test: {

apps/admin/vitest.acceptance.config.ts

@@ -34,11 +34,6 @@ export default defineConfig({
     // suite. Test files and screen helpers import test-lane modules the
     // browser bundler can't process; vitest serves those itself.
     entries: ['src/**/*.{ts,tsx}', '!src/**/*.test.*', '!src/**/*.screen.ts'],
-    // limit-service is CommonJS, and Vite only converts CommonJS while pre-bundling. A
-    // workspace package is treated as source and served raw, where `module` does not exist,
-    // so the import fails and the limiter silently falls back to reporting every host limit
-    // as absent. Force it through the pre-bundler until the package itself is converted.
-    include: ['@tryghost/limit-service'],
   },
   resolve: sharedResolve,
   test: {

ghost/core/core/server/services/limits/counters.ts

@@ -0,0 +1,73 @@
+import type { Counter, Formatter } from '@tryghost/limit-service';
+
+const db = require('../../data/db');
+
+/**
+ * How Ghost counts the things it limits.
+ *
+ * These queries are here rather than in the limit service because they are Ghost's schema:
+ * what a staff user is, that Contributors do not count towards one, that a newsletter only
+ * counts while it is active. The service asks for a number and has no idea where it comes
+ * from, which is the same reason Admin can answer the same questions over HTTP.
+ *
+ * A counted limit with no counter here cannot be applied, and says so rather than going
+ * quiet: the service reports it and Ghost logs it at boot.
+ */
+export const counters: Record<string, Counter> = {
+  members: async ({ transacting } = {}) => {
+    const knex = (transacting ?? db.knex) as ReturnType<typeof require>;
+    const result = await knex('members').count('id', { as: 'count' }).first();
+
+    return Number(result.count);
+  },
+
+  newsletters: async ({ transacting } = {}) => {
+    const knex = (transacting ?? db.knex) as ReturnType<typeof require>;
+    const result = await knex('newsletters')
+      .count('id', { as: 'count' })
+      .where('status', '=', 'active')
+      .first();
+
+    return Number(result.count);
+  },
+
+  emails: async ({ transacting, periodStart } = {}) => {
+    const knex = (transacting ?? db.knex) as ReturnType<typeof require>;
+    const result = await knex('emails')
+      .sum('email_count', { as: 'count' })
+      .where('created_at', '>=', periodStart)
+      .first();
+
+    // A sum over no rows is null, and some drivers return these aggregates as strings.
+    // Either would be compared against the limit as something other than a number.
+    return Number(result.count ?? 0);
+  },
+
+  staff: async ({ transacting } = {}) => {
+    const knex = (transacting ?? db.knex) as ReturnType<typeof require>;
+    const result = await knex('users')
+      .select('users.id')
+      .leftJoin('roles_users', 'users.id', 'roles_users.user_id')
+      .leftJoin('roles', 'roles_users.role_id', 'roles.id')
+      .whereNot('roles.name', 'Contributor')
+      .andWhereNot('users.status', 'inactive')
+      .union([
+        knex('invites')
+          .select('invites.id')
+          .leftJoin('roles', 'invites.role_id', 'roles.id')
+          .whereNot('roles.name', 'Contributor'),
+      ]);
+
+    return result.length;
+  },
+
+  // Uploads compare against the size of the file being uploaded, which the caller passes in
+  // as `currentCount`, so nothing is ever counted here. The limit still needs a counter to
+  // exist, and saying so plainly beats a noop that reads as an oversight.
+  uploads: () => 0,
+};
+
+/** A size reads better as megabytes than as a number of bytes. */
+export const formatters: Record<string, Formatter> = {
+  uploads: (count: number) => `${count / 1000000}MB`,
+};

ghost/core/core/server/services/limits/index.js

@@ -0,0 +1,60 @@
+const errors = require('@tryghost/errors');
+const logging = require('@tryghost/logging');
+const { LimitService } = require('@tryghost/limit-service');
+
+const config = require('../../../shared/config');
+const db = require('../../data/db');
+const { counters, formatters } = require('./counters');
+
+const limitService = new LimitService();
+
+/**
+ * Build this site's limits from the configuration its host supplied.
+ *
+ * Safe to call again whenever that configuration changes: limits are resolved from scratch
+ * and swapped in, and nothing holding a reference to this service needs to know.
+ */
+const init = () => {
+  const hostSettings = config.get('hostSettings') || {};
+
+  const helpLink =
+    hostSettings.billing?.enabled === true && hostSettings.billing?.url
+      ? hostSettings.billing.url
+      : 'https://ghost.org/help/';
+
+  const subscription = hostSettings.subscription
+    ? { startDate: hostSettings.subscription.start, interval: 'month' }
+    : undefined;
+
+  try {
+    limitService.loadLimits({
+      limits: hostSettings.limits || {},
+      counters,
+      formatters,
+      subscription,
+      helpLink,
+      db,
+      errors,
+    });
+  } catch (error) {
+    // Misusing the limit service is a programming error, not a reason to stop a site
+    // booting. Kept from before: configuration problems are reported rather than thrown
+    // now, but a mistake here still must not take a site down.
+    if (error instanceof errors.IncorrectUsageError) {
+      logging.warn(error);
+      return;
+    }
+
+    throw error;
+  }
+
+  // A limit its host is charging for that cannot be applied here is worth saying out loud.
+  // Previously the first one of these took down every limit after it, silently.
+  for (const problem of limitService.problems) {
+    logging.warn(`Host limit "${problem.limit}" was configured but not applied: ${problem.reason}`);
+  }
+};
+
+module.exports = limitService;
+
+module.exports.init = init;