Repository navigation
Convert @tryghost/security to TypeScript - #1081
Conversation
Publish declarations alongside the existing CommonJS entry points so typed consumers can use security helpers without import suppressions. Keep root loading lazy and preserve lib subpaths and token behavior.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughThe security package’s helper modules and entry point now use typed TypeScript. Package scripts and publication settings include compiled CommonJS files and declarations. New configuration and tests cover type checking, runtime behavior, and CommonJS compatibility. Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Feature · Severity of issue fixed: Low Suggested reviewers: Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 13 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly and concisely describes the main change: converting @tryghost/securityto TypeScript.Description check The description directly explains the TypeScript conversion, preserved compatibility, validation, release requirements, and related dependency changes. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 13 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
✨ Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Comment @coderabbitai help to get the list of available commands.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1081 +/- ##
==========================================
- Coverage 98.13% 98.09% -0.04%
==========================================
Files 87 81 -6
Lines 3210 3104 -106
Branches 574 560 -14
==========================================
- Hits 3150 3045 -105
Misses 16 16
+ Partials 44 43 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merge the reset-token parameter rename from main and carry it through the TypeScript implementation and declaration checks. Preserve the fixed-token regression and CommonJS package contract.
ErisDS
left a comment
ErisDS
left a comment
There was a problem hiding this comment.
Review: TypeScript migration gap analysis
Reviewed with independent verification (not just reading the diff):
Verified locally:
- ✅ Full pipeline: build →
test:types→ 42 tests → 100% stmt coverage → lint, all green - ✅ Byte-compatibility: ran main's
tokens.js/secret.js/url.jsagainst the compiled TS output with identical inputs — token bytes identical, reset tokens cross-validate in both directions (old↔new), expired/tampered/garbage paths return identical results, and no-arg calls throw identically (the removedoptions = options || {}was already dead — main throws onundefined.toLocaleLowerCase()too) - ✅ Packed tarball: no
.tssources, notypes/shim, no sourcemaps;index.jslazy getters preserved; deeprequire('@tryghost/security/lib/tokens')works (noexportsfield added — correctly avoids that breaking change) - ✅ Declarations are self-contained:
lib/string.d.tsdoes not leak the@tryghost/stringambient shim (explicit return types mean no import is emitted); onlynode:cryptois referenced - ✅ Strict consumer typecheck passes (strict,
skipLibCheck: false, TS 7.0.2 — note TS 7 has removedesModuleInterop: falseentirely, so that compat matters only for TS ≤6 consumers, which the Ghost checkout validation covered) - ✅
!parts ||guard incomparefixes a latent hole (oldextractcould returnfalse; old code relied on boolean boxing →undefined→isNaN→invalid_expiry) — same observable behaviour, now explicit and tested
No breaking gaps found. Cosmetic notes, none blocking:
__esModule: trueis now emitted onlib/*.js. An interop consumer default-importing a deep path (import t from '@tryghost/security/lib/tokens') previously got the module; now getsundefined. TS consumers get a compile error (good); untyped JS consumers doing this wouldn't. Ghost's call sites were verified, so this is awareness only — maybe worth a line in the release notes for the major bump.types/tryghost-string.d.tswill silently shadow real declarations if@tryghost/stringever ships its own. Suggest aTODO: delete when @tryghost/string publishes typesin the file.vitest.config.tscoverage targeting compiledlib/**/*.jsis the right call (tests the shipped artifact) but looks like a mistake at a glance — a one-line comment would stop a future "fix" back to.ts.safe(string?: string | null)widens the old JSDoc{string}. Fine if it matches real Ghost call sites; tightening to requiredstringwould push null-handling to callers — your call.prepare+pretestmeanstscruns twice in an install+test flow. Harmless, just noise.
Merging on the basis of green CI (Node 22/24, CodeQL, Required checks pass) and the verification above.
Generated by Claude Code
@tryghost/securityhas no declarations, so TypeScript consumers need import suppressions. Convert the package to TypeScript and publish declarations alongside the existing CommonJS JavaScript. Preserve lazy root exports,lib/*entry points, token behavior, and thepasswordHashreset-token API merged in #1083.Validation
corepack pnpm --filter @tryghost/security test— 42 tests pass, including build, typechecks, CommonJS compatibility and the pre-rename token regression.corepack pnpm lintcorepack pnpm format:checkPacked the compiled package into Ghost's invite conversion checkout, removed all four security import suppressions, and updated the reset-token callers to
passwordHash. Full source and test typechecks pass, along with 132 focused consumer tests:pnpm --filter ghost test:types pnpm --filter ghost exec vitest run \ test/unit/server/auth/passwordreset-security-compat.test.js \ test/unit/server/services/invites/ \ test/unit/lib/generate-password.test.ts \ test/unit/server/models/invite.test.js \ test/unit/server/models/user.test.js \ test/unit/frontend/services/routing/controllers/collection.test.js \ test/unit/frontend/services/routing/controllers/channel.test.js \ test/unit/frontend/services/routing/controllers/rss.test.js \ --maxWorkers=2The password-reset compatibility file contains three temporary checks in the validation checkout. They exercise Ghost's real generation/reset orchestration with database doubles, including acceptance of an old token and invalidation after a stored-hash change.
Also verified that Ghost resolves the packed JavaScript and declarations without TypeScript source, CommonJS consumers typecheck with
esModuleInterop: false, and invitation/reset token bytes match the published package.Release
Ship with the next major version of
@tryghost/security. Ghost's adoption must bump the dependency, update the two production reset-token callers and five test fixtures topasswordHash, and remove the four security import suppressions. Those Ghost changes were verified in the disposable checkout and are not included in this Framework PR.