[bkendall]

Prepares the codebase for incremental .js -> .ts migration (Phase 0):

• DefinitelyTyped @types/* packages: Adds @types/compression, @types/concat-stream, @types/destroy, @types/is-url, @types/morgan, @types/on-finished, @types/on-headers, and @types/std-mocks to devDependencies (and updates package-lock.json).
• Ambient declarations for remaining untyped packages: Adds src/modules.d.ts covering the 3 packages without @types/* definitions (basic-auth-connect, router, update-notifier-cjs).
• Shared configuration & request/response types: Extends Configuration, Rewrite, Redirect, and Header (with Matcher requiring at least one of source, glob, or regex) in src/config.ts; exports SuperstaticRequest and SuperstaticResponse from src/options.ts; updates src/middleware/env.ts to use them, default spec to {}, and return early after serving /__/env.json or /__/env.js (noted in changelog.txt).
• Typed middleware test helper: Converts test/helpers.js to test/helpers.ts with typed decorator and SuperstaticMiddlewareHandler exports compatible with both ESM .spec.ts and CommonJS .spec.js suites.

Verification

• tsc --noEmit: 0 errors
• tsc --build tsconfig.publish.json: 0 errors
• npm install --package-lock-only --ignore-scripts: lockfile up to date
• eslint: 0 errors (0 warnings on all added/modified TypeScript files)
• mocha "test/unit/**" "test/integration/**": 234 passing, 15 pending

[gemini-code-assist]

Code Review

This pull request refactors configuration types, exports SuperstaticRequest and SuperstaticResponse interfaces, adds TypeScript declaration files for external modules, and converts the test decorator helper to TypeScript. The review feedback suggests several key improvements: returning early in the env middleware to prevent calling next() after a response is handled, using union types to ensure at least one matcher is defined in Rewrite/Redirect/Header interfaces, making the env property optional in the middleware spec, and passing spec directly in the test decorator to preserve default parameters.

[joehan]

Code Review: PR #610 (refactor: add shared TypeScript types, ambient declarations, and typed test helper)

Strengths & LGTM Aspects

• Strong foundation for the JS to TS migration across superstatic.
• Solid type modeling in src/config.ts for cleanUrls, trailingSlash, headers, redirects, and rewrites.
• Typed helper decorator in test/helpers.ts makes test cases much easier to follow and typesafe.

Key Findings to Address

1. src/middleware/env.ts module export mismatch: export function env mixed with module.exports = env causes export declare function env to be emitted in .d.ts while exports is overwritten at runtime, crashing TypeScript callers with TypeError.
2. Packaging / declaration issue: Moving @types/connect to devDependencies causes emitted .d.ts files that reference connect to fail compilation in downstream projects that do not depend on @types/connect (or degrade superstatic() to any under skipLibCheck).
3. Rewrite type rejects custom rewriters: Firebase Hosting supports function and run rewrites (used in firebase-tools), but Rewrite currently only models destination. Also destination being optional allows targetless rewrites that crash at runtime.
4. MiddlewareOptions.env: string bug: Passing a string filename compiles cleanly, but env.ts does not load disk files and instead spreads string characters into object properties on /__/env.json.
5. Ambient basic-auth-connect declaration: pass?: string is marked optional, but the library throws synchronously if password is not a string.

[joehan]

🔴 [P0 Runtime Crash] Mixed ES export and module.exports breaks TS callers

Line 36 declares export function env(...) while line 63 has module.exports = env;.
When compiling with tsconfig.publish.json, TypeScript emits export declare function env(...) in lib/middleware/env.d.ts (ignoring module.exports), but in the emitted CJS lib/middleware/env.js line 63 overwrites module.exports = env. At runtime, exports.env is undefined.

TypeScript consumers using import { env } from "superstatic/lib/middleware/env" compile with 0 errors and crash at runtime:

TypeError: (0 , env_1.env) is not a function

Consumers using import env from ... fail tsc with TS2349: Type 'typeof import(".../env")' has no call signatures.

Also note line 45 accesses req.superstatic.env without optional chaining (req.superstatic?.env), which will throw TypeError if mounted standalone.

Suggested fix:

function env(spec?: MiddlewareOptions | Record<string, string>): connect.NextHandleFunction {
  // ...
  const config = req.superstatic?.env;
  // ...
}

export = env;

[bkendall]

Addressed in 0c71e06: replaced mixed export with export = env; + export namespace env { export interface EnvOptions ... } so both CommonJS require() and TS import callers work, guarded req.superstatic?.env for standalone mounting, widened handler parameters to connect.IncomingMessage & { superstatic?: Configuration } and ServerResponse & { superstatic?: Responder }, and added standalone test coverage.

[joehan]

🔴 [P1 Packaging / Declaration Defect] @types/connect in devDependencies breaks .d.ts consumers

Emitted declaration files (lib/options.d.ts, lib/server.d.ts, and lib/superstatic.d.ts) import types from "connect". Since connect ships no bundled types, downstream consumers installing superstatic:

1. Under skipLibCheck: false (or strict monorepo builds): fail tsc with TS2307: Cannot find module 'connect' or its corresponding type declarations.
2. Under skipLibCheck: true (e.g. firebase-tools, which does not list @types/connect in its dependencies): unresolved connect imports silently degrade superstatic() and server() return types to any.

Because connect types leak across superstatic's public declaration boundary, @types/connect should be listed in "dependencies", or src/options.ts can declare structural handler types ((req: IncomingMessage, res: ServerResponse, next: (err?: unknown) => void) => void).

[bkendall]

Addressed in 0c71e06: moved @types/connect from devDependencies to dependencies (and updated package-lock.json) so emitted .d.ts declarations referencing connect resolve cleanly for downstream TypeScript consumers.

[joehan]

🔴 [P1 Type Modeling] Rewrite rejects function/run rewrites and allows targetless rewrites

export type Rewrite = Matcher & { destination?: string }; causes two issues:

1. Valid Firebase Hosting rewrites used by firebase-tools (e.g. { source: "/api/**", function: "api" } or { source: "/run/**", run: { serviceId: "svc" } }) fail compilation with TS2353: Object literal may only specify known properties.
2. Because destination is optional, a rewrite with { source: "/foo" } compiles cleanly, but crashes at runtime with HTTP 500 (Error: Unable to find a matching rewriter).

Suggested fix:

export type RewriteTarget =
  | { destination: string }
  | { function: string | { functionId: string; region?: string; pinTag?: boolean } }
  | { run: { serviceId: string; region?: string; pinTag?: boolean } }
  | { dynamicLinks: boolean }
  | ({ destination?: string } & Record<string, unknown>);

export type Rewrite = Matcher & RewriteTarget;

[bkendall]

Addressed in 0c71e06: added the RewriteTarget union supporting destination, function, run, dynamicLinks, and custom rewriter objects, and defined export type Rewrite = Matcher & RewriteTarget;.

[joehan]

🔴 [P1 Type / Runtime Mismatch] MiddlewareOptions.env: string spreads string characters on /__/env.json

env?: string | Record<string, string>; allows passing a file path string. But neither superstatic() nor env.ts loads files from disk when spec.env is a string (the CLI in src/cli/index.ts parses .env.json itself).
If a consumer passes superstatic({ env: ".env.json", config: {} }), env.ts executes Object.assign({}, req.superstatic.env, spec.env) and serves {"0":".","1":"e","2":"n","3":"v",...} at /__/env.json.

env should be typed Record<string, string> (or a loader function).

Also, a few other runtime options are omitted from MiddlewareOptions:

• provider?: Provider (checked in src/superstatic.js)
• allowedHeaders?: string[] (checked in headers.ts)
• compression?: boolean | connect.HandleFunction (checked as function in src/superstatic.js)
• before?: { [key: string]: connect.HandleFunction | connect.HandleFunction[] } (activator supports arrays)

[bkendall]

Addressed in 0c71e06: removed string from MiddlewareOptions.env (now Record<string, string>), added missing runtime options (provider, allowedHeaders, compression, before, and array support on after), and exported shared provider/responder directive types from src/options.ts so downstream modules can share a single canonical definition.

[joehan]

🟡 [P2 Ambient Declaration] basic-auth-connect requires pass: string

In basic-auth-connect, line 51 explicitly throws new Error("password argument required") if typeof password !== "string".
Typing pass?: string as optional hides runtime server crashes when colon-less credentials are provided to basic auth.

Suggested change

	pass?: string,
	(user: string, pass: string): connect.NextHandleFunction;

[bkendall]

Addressed in 0c71e06: changed pass from optional to required pass: string in the basic-auth-connect declaration to match its runtime validation.

[joehan]

🟡 [P2 Test Helper] Missing return drops promises from async middleware

middleware((spec ?? {}) as Spec)(req, res, ...) drops the return value. For async middlewares (like files or redirects), any returned Promise is discarded. Adding return preserves returned promises in tests.

Also, spec ?? {} masks testing default parameters when spec is omitted, and as Spec bypasses type checking for required properties.

[bkendall]

Addressed in 0c71e06: added return before middleware(...)(...) (with handler return type : unknown) so promises from async middlewares are preserved in tests. Kept spec ?? {} in helpers.decorator for now because unconverted JS middleware factories (headers, protect, missing, not-found) still access spec.<prop> without default parameters when tests omit spec; we can drop spec ?? {} once those middlewares migrate.

[joehan]

🟡 Nit: Assert rendered body for /__/env.js

The new test checks status code and Content-Type, but does not assert that the response body contains window.__env = .... Adding a body assertion ensures the template payload is formatted as expected.

[bkendall]

Addressed in 0c71e06: added a rendered body assertion verifying the interpolated templates/env.js.template output on /__/env.js.