Skip to content

Conversation

@SethFalco
Copy link
Member

@SethFalco SethFalco commented Oct 17, 2025

Adds type checking via TypeScript. As we don't officially maintain a TypeScript interface for JSON Resume, I just use any for that. It's a little lazy, but I'll evaluate this later.

Also adds a test task to CI, which will enforce types in pull requests. 💪(•ᴗ•💪)

Chores

  • Instead of calling await right away when loading files, I load them first, do all other logic, then call await when they're actually needed. This difference is not perceivable, but it is more logical.

Summary by CodeRabbit

  • New Features

    • Integrated automated type checking into pull request workflows.
  • Chores

    • Added TypeScript tooling, configuration, and a new type-check script for static validation.
  • Refactor

    • Improved promise handling in the render flow and updated a JSDoc parameter annotation for clarity.

@coderabbitai
Copy link

coderabbitai bot commented Oct 17, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Adds TypeScript tooling and CI type-checking: a new GitHub Actions "test" job runs yarn and yarn run typecheck; adds tsconfig.json, TypeScript and type definitions as devDependencies, and a small refactor/JSDoc change in index.js.

Changes

Cohort / File(s) Summary
GitHub Actions Workflow
.github/workflows/pulls.yml
Adds a new test job (runs on ubuntu-latest) that enables corepack, sets up Node.js 22, installs dependencies with Yarn, and runs yarn run typecheck.
TypeScript Configuration
tsconfig.json
New file with compilerOptions: strict, noEmit, ES2021 target/lib, allowJs/checkJs, resolveJsonModule, skipLibCheck; excludes .yarn/ and node_modules/.
Package Configuration
package.json
Adds typecheck script (tsc --noEmit) and devDependencies: typescript, @types/node, @types/html-minifier.
Render Function Update
index.js
JSDoc param updated from {Object} to {any}; refactors Promise.all usage by assigning to a loading Promise and awaiting it later—behavior and final [css, template] remain the same.

Sequence Diagram(s)

sequenceDiagram
  autonumber
  participant PR as Pull Request
  participant GH as GitHub Actions (pulls.yml)
  participant Runner as ubuntu-latest
  participant Registry as npm Registry
  PR->>GH: triggers workflow
  GH->>Runner: run "build" job (existing)
  GH->>Runner: run new "test" job
  Runner->>Runner: actions/checkout@v5
  Runner->>Runner: corepack enable
  Runner->>Registry: setup-node@v6 (Node 22, npm config)
  Runner->>Runner: yarn install
  Runner->>Runner: yarn run typecheck (tsc --noEmit)
  alt typecheck success
    Runner->>GH: job passes
  else typecheck failure
    Runner->>GH: job fails
  end
Loading
sequenceDiagram
  autonumber
  participant Renderer as render(resume)
  participant Loader as Promise resources
  Renderer->>Loader: create loading = Promise.all([css, template])
  Renderer-->>Loader: do other sync work
  Loader->>Renderer: await loading -> [css, template]
  Renderer->>Renderer: continue to render using css and template
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰 I nibble on tsconfig nights,

yarn hums as the CI lights,
Promise.all now waits with care,
JSDoc softly breathes "any" there,
Hops of green on type-checked flights. 🥕

Pre-merge checks and finishing touches

✅ 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 "chore: add typechecking via typescript" accurately and concisely describes the primary objective of the pull request, which is to introduce TypeScript-based type checking to the project. The changes across multiple files—including the addition of tsconfig.json, TypeScript dependencies in package.json, a new type-checking CI job, and JSDoc updates—all directly support this single coherent goal. The title is clear, specific, and avoids vague terminology, making it easy for someone scanning the commit history to understand the main change.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 764c446 and 77700f1.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • .github/workflows/pulls.yml (1 hunks)
  • index.js (2 hunks)
  • package.json (2 hunks)
  • tsconfig.json (1 hunks)

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

@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

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fb3911c and 764c446.

⛔ Files ignored due to path filters (1)
  • yarn.lock is excluded by !**/yarn.lock, !**/*.lock
📒 Files selected for processing (4)
  • .github/workflows/pulls.yml (1 hunks)
  • index.js (2 hunks)
  • package.json (2 hunks)
  • tsconfig.json (1 hunks)
🔇 Additional comments (4)
package.json (2)

31-31: Script configuration looks good, but CI workflow needs update.

The types script correctly runs TypeScript type checking without emitting output. However, the CI workflow calls typecheck instead of types, which will cause CI to fail (already flagged in the workflow file).


44-47: LGTM!

The TypeScript and type definition dependencies are appropriate for enabling type checking in this project.

index.js (2)

79-79: Temporary use of any type is reasonable given constraints.

While any bypasses type safety, the PR description explains this is intentional due to the lack of an official TypeScript interface for JSON Resume. Consider referencing a type definition from a community package (e.g., @types/resume-schema or similar) if one becomes available.


83-86: Loading refactor is functionally correct.

The refactor defers awaiting file I/O until the loaded values are actually needed. While this doesn't provide measurable performance benefits (since the intervening code at lines 88-115 is synchronous), the code organization is logical and maintains correctness.

Also applies to: 117-117

@SethFalco SethFalco merged commit 10fc89d into jsonresume:main Oct 17, 2025
2 of 3 checks passed
@SethFalco SethFalco deleted the typechecking branch October 17, 2025 12:05
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.

1 participant