Skip to content

Conversation

@w3nl
Copy link
Contributor

@w3nl w3nl commented Sep 12, 2025

Summary by CodeRabbit

  • New Features
    • Added Deno support and configuration, including lint and type checks.
    • Enabled publishing to JSR alongside npm.
    • Improved TypeScript tooling with type stub generation and updated configs.
  • Changes
    • setupServer and unauthorized handlers are now synchronous (no Promise).
    • openAPI now returns an object containing openAPISpecification.
  • Bug Fixes
    • Ensured reliable ETag generation by importing Buffer explicitly.
  • Chores
    • Enhanced CI: added Deno checks and type-lint step.
    • Updated publish workflow to sync versions and publish to JSR.

@w3nl w3nl requested a review from Copilot September 12, 2025 12:11
@coderabbitai
Copy link

coderabbitai bot commented Sep 12, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds Deno/JSR support and CI updates (lint, check, publish). Introduces jsr.json and deno.json, and augments publishing to also publish to JSR. Enhances TS/JS configs, adds type-stub generation, and embeds ts-self-types across source files. Minor API shifts: some functions become synchronous; small router/Ajv defaulting change.

Changes

Cohort / File(s) Summary of changes
CI: Lint
.github/workflows/lint.yml
Runs npm lint + types, installs Deno, adds deno lint and deno task check; minor quoting tweak.
CI: Publish
.../npm-publish.yml
Adds permissions; sets version in package.json, lockfile, jsr.json, deno.json; commits them; runs npm publish then npx jsr publish.
Deno/JSR Manifests
deno.json, jsr.json
New manifests with name/version/license/exports; deno.json adds compilerOptions and task “check”.
TS/JS Config
tsconfig.json, jsconfig.json
Enables ESNext libs, NodeNext resolution, baseUrl, rootDirs, node types, resolveJsonModule, esModuleInterop; includes src/types.
Package & Scripts
package.json, scripts/create-type-stubs.js
Adds lint:types; updates build:types to run tsc + stub generator; adds source/module fields; bumps @types/node; adds deno devDep; new script writes stubs in types/.js re-exporting src/.js.
Typing directives (core)
src/api.js, src/openapi.js, src/operation-ids.js, src/params.js, src/error-status.js, src/express-callback.js, src/types.js
Adds ts-self-types headers; minor JSDoc typedef/import path adjustments; api.js imports Buffer; types.js introduces URLStatic typedef and casts URL.
Handlers
src/handlers/not-found.js, src/handlers/request-validation.js, src/handlers/response-validation.js, src/handlers/unauthorized.js
Adds ts-self-types; marks unused params with underscores; unauthorized becomes synchronous.
Router
src/router.js, src/router.test.js
Adds ts-self-types; typedef import path tweaks; sets default customizeAjv that adds formats; minor formatting; tests adjust unused param names/async.
Server
src/server.js
Adds ts-self-types; typedefs updated; setupServer changed from async to sync with defaulted params; return now plain object (no Promise).

Sequence Diagram(s)

sequenceDiagram
  autonumber
  actor Dev as Developer
  participant GH as GitHub Actions
  participant Node as Node Lint
  participant Deno as Deno Lint/Check

  Dev->>GH: Push / PR
  GH->>Node: npm ci && npm run lint
  GH->>Node: npm run lint:types
  GH->>Deno: setup-deno v1 (v1.x)
  GH->>Deno: deno lint
  GH->>Deno: deno task check
  GH-->>Dev: CI status
Loading
sequenceDiagram
  autonumber
  actor Maint as Maintainer
  participant GH as GitHub Actions
  participant Pub as npm publish
  participant JSR as jsr publish

  Maint->>GH: Tag push (vX.Y.Z)
  GH->>GH: Set version in package.json, lockfile, jsr.json, deno.json
  GH->>Pub: npm publish
  GH->>JSR: npx jsr publish
  GH-->>Maint: Release complete
Loading
sequenceDiagram
  autonumber
  participant App as Consumer Code
  participant Srv as setupServer
  participant Rt as Router
  Note over Srv: Now synchronous
  App->>Srv: setupServer({ apis, origin?, ... })
  Srv->>Rt: create router (customizeAjv default adds formats)
  Srv-->>App: { app }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Pre-merge checks (3 passed)

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "Add package to JSR" is concise and accurately reflects the primary intent of the changeset (adding a jsr.json manifest and integrating JSR publishing support alongside related package/CI updates), so it clearly summarizes the main change for a reviewer. It is short, specific, and avoids noise or vague phrasing.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Poem

A rabbit taps keys with a deno-like grin,
Lints on the left, types checking within.
JSR sails after npm’s push,
Stubs sprout quick with a gentle whoosh.
Async hops slow? We switch to sync—
Carrots committed, tags in sync! 🥕✨

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.

✨ Finishing touches
  • 📝 Generate Docstrings
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch feature/jsr-deno

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR prepares the package for publication to JSR (JavaScript Registry) by adding necessary configuration files and performing comprehensive code style modernization. The changes include updating dependencies, reformatting code to use a consistent style without semicolons, enhancing type definitions, and adding JSR-specific configuration.

  • Added JSR configuration files (jsr.json, deno.json) and updated package.json for JSR compatibility
  • Modernized code formatting by removing semicolons and updating to a consistent style
  • Enhanced type definitions and added TypeScript configuration improvements

Reviewed Changes

Copilot reviewed 34 out of 38 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
jsr.json New JSR registry configuration file
deno.json New Deno configuration for JSR compatibility
package.json Updated dependencies, scripts, and package configuration for JSR
src/*.js Code style modernization - removed semicolons, updated formatting
eslint.config.js New ESLint configuration replacing .eslintrc
.github/workflows/* Updated CI/CD workflows for newer Node versions and JSR publishing

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

poweredBy = 'TroJS',
version = '1.0.0',
middleware = [],
export const setupServer = ({
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setupServer function signature has changed from async to sync, removing the Promise return type, but the function is still exported as if it returns a Promise. This is a breaking API change that could cause issues for existing consumers expecting an async function.

Copilot uses AI. Check for mistakes.
@@ -1,4 +1,5 @@
export const responseValidation = (context, request, response) => {
/* @ts-self-types="../../types/handlers/response-validation.d.ts" */
export default (logger) => (context, request, response) => {
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The response validation handler has changed from a direct export to a factory function that takes a logger parameter. This is a breaking change that will affect any code importing this handler directly.

Copilot uses AI. Check for mistakes.
return acc;
}, {});
if (!paramName) {
return undefined
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for handling missing parameters has changed significantly. The function now returns undefined for missing parameters without defaults/examples, but this undefined value gets filtered out later. This could break existing code that expects all parameters to be included in the result object with example values.

Suggested change
return undefined
return { name, value: undefined }

Copilot uses AI. Check for mistakes.
@w3nl w3nl changed the base branch from v1.x to main September 12, 2025 12:19
@w3nl w3nl requested a review from Copilot September 12, 2025 12:19
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 21 out of 23 changed files in this pull request and generated no new comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@w3nl w3nl requested a review from Copilot September 12, 2025 12:20
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 21 out of 23 changed files in this pull request and generated 1 comment.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment on lines +52 to +55
customizeAjv: customizeAjv || ((originalAjv) => {
addFormats(originalAjv)
return originalAjv
}
})
Copy link

Copilot AI Sep 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conditional assignment with a fallback function creates unnecessary complexity. Consider using a default parameter in the function signature instead: customizeAjv = (originalAjv) => { addFormats(originalAjv); return originalAjv }

Copilot uses AI. Check for mistakes.
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (4)
src/error-status.js (1)

23-25: Add safe fallback when non-Error is thrown.

throw 'oops' (string) will make .find(...).status crash. Provide a default.

-export default (error) =>
-  errorCodesStatus.find((errorCode) => error instanceof errorCode.type)
-    .status
+export default (error) =>
+  (errorCodesStatus.find((e) => error instanceof e.type)?.status) ?? 500
src/api.js (1)

45-59: Fix lint: add explicit param and return types on public API

Inline the destructured constructor param type and declare the setup() return type to satisfy “no-slow-types”.

-  constructor ({
+  constructor (/** @type {ApiSchema} */ {
     version,
     specification,
     controllers,
     apiRoot,
     strictSpecification,
     errorDetails,
     logger,
     meta,
     securityHandlers,
     swagger,
     apiDocs,
     ajvOptions,
     middleware = []
   }) {
@@
-  setup () {
+  /** @returns {import('express').Router} */
+  setup () {

Also applies to: 75-75

src/openapi.js (1)

8-13: Fix lint: attach explicit param/return types to the public API (and avoid destructured-param JSDoc pitfalls).

Your linter isn’t picking up the @type on the arrow. Switch to @param/@returns and destructure inside the function to ensure binding. Also, this keeps types explicit without relying on self-types.

-/** @type {(params: OpenAPIParams) => Promise<{ openAPISpecification: object }>} */
-export const openAPI = async ({ file, base = import.meta.url }) => {
+/**
+ * @param {OpenAPIParams} params
+ * @returns {Promise<{ openAPISpecification: object }>}
+ */
+export const openAPI = async (params) => {
+  const { file, base = import.meta.url } = params
   const fileUrl = new URL(file, base)
   const openAPISpecification = JSON.parse(await readFile(fileUrl, 'utf8'))
   return { openAPISpecification }
 }
.github/workflows/npm-publish.yml (1)

49-64: Fix tag handling: git reset uses the sanitized TAG (without v), breaking when tags are prefixed (common).

Use the raw tag for Git refs and the sanitized tag only for the version numbers. Also, consider set -euo pipefail to fail fast.

Proposed replacement for the “Set version from tag” run script (full block):

#!/bin/bash
set -euo pipefail

git config user.name "github-actions[bot]"
git config user.email "github-actions[bot]@users.noreply.github.com"

RAW_TAG="${GITHUB_REF#refs/tags/}"     # e.g., v3.4.1
TAG="${RAW_TAG#v}"                     # e.g., 3.4.1

git fetch origin "${{ steps.get_branch.outputs.branch }}"
git checkout "${{ steps.get_branch.outputs.branch }}"

# Ensure workspace matches the released tag commit
git reset --hard "refs/tags/${RAW_TAG}"

# Bump versions to the numeric tag (no 'v')
npm version "$TAG" --no-git-tag-version
jq '.version = "'$TAG'"' jsr.json  > jsr.json.tmp  && mv jsr.json.tmp  jsr.json
jq '.version = "'$TAG'"' deno.json > deno.json.tmp && mv deno.json.tmp deno.json

git add package.json package-lock.json jsr.json deno.json
git commit -m "ci: set version to $TAG [skip ci]" || true
git push origin "${{ steps.get_branch.outputs.branch }}"
♻️ Duplicate comments (2)
src/params.js (1)

32-46: Behavior on missing params: confirm intent to omit keys.

Returning undefined here (and filtering) omits missing params entirely. If callers expect all declared params present (possibly undefined), return an entry and keep it through reduce.

-      if (!paramName) {
-        return undefined
-      }
+      if (!paramName) {
+        return { name, value: undefined }
+      }

Note: keeping this change requires preserving those entries through the .filter(Boolean) step (it’s fine as-is because objects are truthy).

src/router.js (1)

52-56: Prefer default param over runtime fallback for customizeAjv.

This reduces branching and keeps intent at the boundary. This echoes prior feedback.

-export const setupRouter = ({
+export const setupRouter = ({
   openAPISpecification,
   controllers,
   apiRoot,
   strictSpecification,
   errorDetails,
   logger,
   meta,
   securityHandlers = [],
   ajvOptions = {},
-  customizeAjv,
+  customizeAjv = (originalAjv) => { addFormats(originalAjv); return originalAjv },
   mock
 }) => {
   const api = new OpenAPIBackend({
     definition: openAPISpecification,
     apiRoot,
     strict: strictSpecification,
     ajvOpts: ajvOptions,
-    customizeAjv: customizeAjv || ((originalAjv) => {
-      addFormats(originalAjv)
-      return originalAjv
-    })
+    customizeAjv
   })
🧹 Nitpick comments (16)
src/handlers/response-validation.js (2)

8-28: Harden validation result handling (avoid truthy-object/boolean ambiguity).

If validateResponse returns false (Ajv-style) or an object with errors, the current if (valid && valid.errors) can miss failures. Recommend normalizing the check.

-  const valid = context.api.validateResponse(
+  const result = context.api.validateResponse(
     context.response,
     context.operation
   )
-  if (valid && valid.errors) {
+  const hasErrors =
+    result === false || (Array.isArray(result?.errors) && result.errors.length > 0)
+  if (hasErrors) {
     if (logger) {
       logger.error({
         message: 'Response validation failed',
-        errors: valid.errors,
+        errors: result?.errors ?? [],
         operation: context.operation,
         statusCode: response.statusCode,
         response: context.response
       })
     }
     return response.status(502).json({
-      errors: valid.errors,
+      errors: result?.errors ?? [],
       status: 502,
       timestamp: new Date(),
       message: 'Bad response'
     })
   }

34-37: Accept negotiation should tolerate lists/params.

Accept often contains multiple types with q-params. Consider includes('application/json') instead of strict equality.

-  const contentType = request?.headers?.accept ?? 'application/json'
-  if (contentType === 'application/json') {
+  const accept = request?.headers?.accept ?? 'application/json'
+  if (accept.includes('application/json')) {
     return response.json(context.response)
   }
deno.json (1)

1-14: JSR exports shape: prefer explicit subpath map and add handy tasks.

Verified: versions across package.json, jsr.json, deno.json match (3.4.1).

 {
     "name": "@trojs/openapi-server",
     "version": "3.4.1",
     "license": "MIT",
-    "exports": "./src/server.js",
+    "exports": {
+        ".": "./src/server.js"
+    },
     "compilerOptions": {
         "lib": [
             "esnext"
         ]
     },
     "tasks": {
-        "check": "deno check src/server.js"
+        "check": "deno check src/server.js",
+        "lint": "deno lint",
+        "fmt": "deno fmt",
+        "fmt:check": "deno fmt --check"
     }
 }
src/operation-ids.js (1)

10-17: Filter out null/undefined operationIds.

Current implementation returns nulls for non-HTTP keys; prune them before returning.

 export const operationIds = ({ specification }) =>
   Object.values(specification.paths)
     .map((path) =>
       Object.entries(path).map(([operation, data]) =>
         operations.includes(operation) ? data.operationId : null
       )
     )
-    .flat()
+    .flat()
+    .filter((v) => typeof v === 'string' && v.length > 0)
.github/workflows/lint.yml (1)

15-33: Nice Deno integration; consider PR trigger and fmt check.

Add pull_request trigger and a deno fmt --check step for formatting parity.

-name: Node Lint
+name: Lint

-on: [push]
+on: [push, pull_request]

       - name: Deno lint
         run: deno lint
+      - name: Deno fmt check
+        run: deno fmt --check
       - name: Deno check
         run: deno task check
tsconfig.json (1)

23-34: Include "types" in rootDirs for better editor resolution of stubs.

Helps TS/IDE resolve paths when using generated d.ts alongside src with ts-self-types.

-        "rootDirs": [
-            "src"
-        ],
+        "rootDirs": [
+            "src",
+            "types"
+        ],

Optional: ensure your TypeScript version supports noUncheckedSideEffectImports. If unsure, print it in CI (npx tsc -v) or pin a compatible version.

src/api.js (2)

60-73: Use nullish coalescing consistently for boolean defaults

Avoids surprising behavior with falsey non-null values; align style with nearby fields.

-    this.errorDetails = errorDetails || false
+    this.errorDetails = errorDetails ?? false

4-4: Prefer short, stable (weak) ETags using a hash

Base64 of the full JSON inflates headers and can change with key order. A weak SHA-256 keeps it small and stable.

-import { Buffer } from 'node:buffer'
+import { Buffer } from 'node:buffer'
+import { createHash } from 'node:crypto'
@@
-    // Generate an ETag for the specification (simple hash or JSON string)
-      const apiDocsString = JSON.stringify(this.specification)
-      const etag = `"${Buffer.from(apiDocsString).toString('base64')}"`
+      // Weak ETag from a hash of the JSON payload
+      const apiDocsString = JSON.stringify(this.specification)
+      const hash = createHash('sha256').update(apiDocsString).digest('base64url')
+      const etag = `W/"${hash}"`
@@
-        const ifNoneMatchHeader = request.headers['if-none-match']
+        const ifNoneMatchHeader = request.headers['if-none-match']
         if (ifNoneMatchHeader) {
           const etags = ifNoneMatchHeader.split(',').map((tag) => tag.trim())
-          if (etags.includes('*') || etags.includes(etag)) {
+          if (etags.includes('*') || etags.includes(etag)) {
             response.status(304).end()
             return
           }
         }

Also applies to: 86-105

src/express-callback.js (2)

65-96: Harden error handling for non-Error throws

Normalize unknown throws to Error to keep status/message typing consistent.

-      } catch (error) {
-        const errorCodeStatus = getStatusByError(error)
+      } catch (error) {
+        const e = error instanceof Error ? error : new Error(String(error))
+        const errorCodeStatus = getStatusByError(e)
@@
-          logger.error(error)
+          logger.error(e)
         } else {
-          logger.warn(error)
+          logger.warn(e)
         }
@@
-                message: error.message,
-                value: error.valueOf(),
-                type: error.constructor.name
+                message: e.message,
+                value: e.valueOf(),
+                type: e.constructor.name
@@
-            message: error.message
+            message: e.message
@@
-          message: error.message
+          message: e.message

57-63: Consider redacting large/sensitive bodies in debug logs

Logging full request.body can be heavy or leak PII; consider size limit or allowlist fields.

src/types.js (1)

2-12: Relax URLStatic to avoid overstating Node capabilities

In Node, parse/createObjectURL/revokeObjectURL may be absent; mark optional to keep typings portable across Node/Deno/browsers.

- * canParse(url: string | URL, base?: string | URL): boolean
- * createObjectURL(obj: Blob | unknown): string
- * parse(url: string | URL, base?: string | URL): URL | null
- * revokeObjectURL(url: string): void
+ * canParse?: (url: string | URL, base?: string | URL) => boolean
+ * createObjectURL?: (obj: Blob | unknown) => string
+ * parse?: (url: string | URL, base?: string | URL) => URL | null
+ * revokeObjectURL?: (url: string) => void

Also applies to: 21-21

package.json (1)

39-42: Add an exports map for clearer ESM entry and types

Improves Node/TS resolution and tooling, and aligns with JSR packaging.

   "main": "src/server.js",
   "source": "src/server.js",
   "module": "src/server.js",
+  "exports": {
+    ".": {
+      "types": "./types/server.d.ts",
+      "import": "./src/server.js"
+    }
+  },
src/openapi.js (1)

5-6: Widen base type to accept URL too.

new URL(file, base) accepts string | URL. Minor ergonomics improvement.

- * @typedef {{ file: string; base?: string }} OpenAPIParams
+ * @typedef {{ file: string; base?: string | URL }} OpenAPIParams
src/server.js (2)

61-74: Clean up JSDoc: stray @param and duplicate typedefs.

The @param {string=} params.version line here is out of place and ApiSchema/Express are already typedef’d above. Consolidate to reduce noise and avoid confusion.

 /**
- * @param {string=} params.version
- * @typedef {import('../src/api.js').ApiSchema} ApiSchema
- * @typedef {import('express').Express} Express
  * @typedef {object} SetupServerParams
  * @property {ApiSchema[]} apis
  * @property {string=} origin
  * @property {string=} staticFolder
  * @property {SentryConfig=} sentry
  * @property {string=} poweredBy
  * @property {string=} version
  * @property {any[]=} middleware
  * @property {string|number=} maximumBodySize
  */

78-87: Confirm intent: async → sync is a breaking type change.

Runtime await setupServer() still works (awaiting a plain object), but TypeScript/typedef consumers now see a sync return. Either:

  • keep it sync and document as breaking (semver-major), or
  • keep backward compatibility by returning a Promise.

Option A (compat):

-/**
- * @param {SetupServerParams} params
- * @returns {{ app: Express }}
- */
-export const setupServer = ({
+/**
+ * @param {SetupServerParams} params
+ * @returns {Promise<{ app: Express }>}
+ */
+export const setupServer = async ({
   ...
 }) => {
   ...
-  return { app }
+  return { app }
 }

If you choose to keep it sync, update the public docs and release notes accordingly.

.github/workflows/npm-publish.yml (1)

21-24: Leverage provenance and clarify auth for JSR publish.

  • You granted id-token: write; enable provenance on npm publish.
  • Ensure jsr publish can auth (OIDC or token). If relying on OIDC, you’re fine; otherwise pass JSR_AUTH_TOKEN secret.
       - run: npm publish --access public
         env:
-            NODE_AUTH_TOKEN: ${{ secrets.npm_token }}
+            NODE_AUTH_TOKEN: ${{ secrets.npm_token }}
+            NPM_CONFIG_PROVENANCE: true
       - run: npx jsr publish
+        env:
+            # If you use PAT-based auth instead of OIDC, uncomment:
+            # JSR_AUTH_TOKEN: ${{ secrets.JSR_TOKEN }}

Also applies to: 67-69

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2c98c0d and 8c005b3.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (22)
  • .github/workflows/lint.yml (1 hunks)
  • .github/workflows/npm-publish.yml (2 hunks)
  • deno.json (1 hunks)
  • jsconfig.json (1 hunks)
  • jsr.json (1 hunks)
  • package.json (1 hunks)
  • scripts/create-type-stubs.js (1 hunks)
  • src/api.js (1 hunks)
  • src/error-status.js (1 hunks)
  • src/express-callback.js (3 hunks)
  • src/handlers/not-found.js (1 hunks)
  • src/handlers/request-validation.js (1 hunks)
  • src/handlers/response-validation.js (1 hunks)
  • src/handlers/unauthorized.js (1 hunks)
  • src/openapi.js (1 hunks)
  • src/operation-ids.js (1 hunks)
  • src/params.js (1 hunks)
  • src/router.js (4 hunks)
  • src/router.test.js (3 hunks)
  • src/server.js (3 hunks)
  • src/types.js (1 hunks)
  • tsconfig.json (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/handlers/unauthorized.js (1)
src/server-no-auth.test.js (1)
  • customHandler (48-60)
src/handlers/request-validation.js (1)
src/express-callback.test.js (1)
  • context (60-64)
src/express-callback.js (3)
src/express-callback.test.js (2)
  • context (60-64)
  • logger (50-54)
src/params.js (2)
  • parseParams (12-50)
  • parseParams (12-50)
src/error-status.js (1)
  • error (23-25)
src/server.js (2)
src/server.test.js (1)
  • setupServer (83-88)
src/server-no-auth.test.js (1)
  • setupServer (74-79)
🪛 GitHub Actions: Node Lint
src/api.js

[error] 45-45: no-slow-types: missing explicit type in the public API. This symbol is missing an explicit type.


[error] 75-75: no-slow-types: missing explicit return type in the public API. This function is missing an explicit return type.

src/openapi.js

[error] 9-9: no-slow-types: missing explicit type in the public API. This symbol is missing an explicit type.


[error] 9-9: no-slow-types: missing explicit return type in the public API. This function is missing an explicit return type.

src/server.js

[error] 78-78: no-slow-types: missing explicit return type in the public API. This function is missing an explicit return type.


[error] 78-78: no-slow-types: missing explicit type in the public API. This symbol is missing an explicit type.

🔇 Additional comments (12)
src/handlers/response-validation.js (1)

1-1: LGTM: ts-self-types directive is correctly placed.

src/handlers/not-found.js (1)

1-9: LGTM: unused-arg underscore + ts-self-types.

Signature and behavior remain consistent with tests.

src/api.js (1)

122-124: Confirm handleRequest arg order matches openapi-backend

You pass (request, request, response). If openapi-backend expects (contextLike, req, res) this is fine; otherwise please adjust.

scripts/create-type-stubs.js (1)

1-15: LGTM — simple, robust stub generation

Top-level await, mkdirp, and idempotent writes look good.

src/handlers/request-validation.js (1)

1-10: LGTM — consistent param underscore and payload

Matches handler shape used elsewhere and tests.

jsconfig.json (1)

3-27: Config aligns with new types flow

Looks fine. If both tsconfig.json and jsconfig.json coexist, ensure editors prefer tsconfig for emits and jsconfig for JS type-check only.

src/handlers/unauthorized.js (1)

1-10: LGTM — sync handler is fine

No async work; underscore params clarify intent. Tests cover 401 path.

jsr.json (1)

1-6: JSR manifest — exported entry exists (Node ESM); confirm runtime compatibility

Verified: src/server.js exists and is importable as Node ESM (script output: "OK: src/server.js exists" / "OK: Node ESM import works"). Since ./src/server.js pulls in Node/Express, confirm consumers are Node (or Deno with Node-compat). Add a short README note about supported runtimes and consider adding a Deno/browser-friendly export in jsr.json if broader compatibility is desired.

src/router.test.js (3)

12-14: LGTM: underscore unused params.

Matches the codebase convention and quiets linters.


39-41: LGTM: inner test no longer async.

No await inside; removing async avoids an implicit Promise.


52-53: LGTM: AJV keyword validate signature.

Ignoring _schema reads cleaner and keeps types aligned.

src/router.js (1)

86-87: LGTM: notImplemented mock extraction formatting.

Readability improved; logic unchanged.

Comment on lines 23 to 25
export const makeExpressCallback
= ({ controller, specification, errorDetails, logger, meta, mock }) =>
= ({ controller, specification, errorDetails, logger, meta, mock }) =>
/**
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Avoid crash when logger is undefined

Default to console; current code would throw on logger.debug/error/warn when not provided.

-export const makeExpressCallback
-  = ({ controller, specification, errorDetails, logger, meta, mock }) =>
+export const makeExpressCallback
+  = ({ controller, specification, errorDetails, logger = console, meta, mock }) =>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
export const makeExpressCallback
= ({ controller, specification, errorDetails, logger, meta, mock }) =>
= ({ controller, specification, errorDetails, logger, meta, mock }) =>
/**
export const makeExpressCallback
= ({ controller, specification, errorDetails, logger = console, meta, mock }) =>
/**
🤖 Prompt for AI Agents
In src/express-callback.js around lines 23 to 25, the exported
makeExpressCallback assumes logger is provided and will crash when calling
logger.debug/error/warn if undefined; change the implementation to default the
logger to the global console (either via a default parameter or by assigning
const logger = passedLogger || console at the top of the function) and then use
that logger variable for all logging calls so the code safely falls back to
console when no logger is supplied.

Comment on lines +76 to +78
/** @type {(params: SetupServerParams) => { app: Express }} */

export const setupServer = ({
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix lint: public API missing explicit types (JSDoc not bound due to blank line).

Remove the blank line and switch to @param/@returns so the linter recognizes the signature.

-/** @type {(params: SetupServerParams) => { app: Express }} */
-
-export const setupServer = ({
+/**
+ * @param {SetupServerParams} params
+ * @returns {{ app: Express }}
+ */
+export const setupServer = ({
   apis,
   origin = '*',
   staticFolder,
   sentry,
   poweredBy = 'TroJS',
   version = '1.0.0',
   middleware = [],
   maximumBodySize = undefined
 }) => {
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
/** @type {(params: SetupServerParams) => { app: Express }} */
export const setupServer = ({
/**
* @param {SetupServerParams} params
* @returns {{ app: Express }}
*/
export const setupServer = ({
apis,
origin = '*',
staticFolder,
sentry,
poweredBy = 'TroJS',
version = '1.0.0',
middleware = [],
maximumBodySize = undefined
}) => {
🧰 Tools
🪛 GitHub Actions: Node Lint

[error] 78-78: no-slow-types: missing explicit return type in the public API. This function is missing an explicit return type.


[error] 78-78: no-slow-types: missing explicit type in the public API. This symbol is missing an explicit type.

🤖 Prompt for AI Agents
In src/server.js around lines 76 to 78, the JSDoc for the exported setupServer
function is separated by a blank line so the linter doesn't bind the signature;
remove the blank line and replace the current type-only JSDoc with an explicit
@param and @returns block directly above the setupServer declaration (e.g. a
JSDoc that documents the params as SetupServerParams and returns an object with
app: Express), ensuring the comment immediately precedes the export so the
linter recognizes the public API types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants