Converted @tryghost/tpl to TypeScript#720
Conversation
This comment was marked as low quality.
This comment was marked as low quality.
EvanHahn
force-pushed
the
convert-tpl-to-typescript
branch
from
a548827 to
6a562dd
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #720 +/- ##
==========================================
- Coverage 98.05% 98.04% -0.01%
==========================================
Files 85 84 -1
Lines 2770 2759 -11
Branches 508 506 -2
==========================================
- Hits 2716 2705 -11
Misses 12 12
Partials 42 42 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR converts the @tryghost/tpl workspace package from JavaScript to TypeScript and starts publishing generated .d.ts type definitions alongside the compiled JavaScript, while aiming to preserve the existing CommonJS consumption pattern.
Changes:
- Add TypeScript project configuration for
packages/tpl(build + test typecheck). - Replace the implementation entrypoints with TypeScript sources and emit
.js/.d.tsviatsc. - Update package metadata/scripts to build before tests and publish type declarations.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Updates the lockfile for packages/tpl dev dependency changes (TypeScript + Node types). |
| packages/tpl/vitest.config.ts | Adds per-package Vitest config merged from the repo root configuration. |
| packages/tpl/tsconfig.test.json | Adds a test-specific TS config enabling Node + Vitest global typings. |
| packages/tpl/tsconfig.json | Adds the package TS build configuration extending the shared packages/tsconfig.json. |
| packages/tpl/test/tpl.test.ts | Migrates tests from CommonJS require to ES imports and minor const cleanups. |
| packages/tpl/package.json | Publishes generated .d.ts, adds build/prepare scripts, and adds TS-related dev deps. |
| packages/tpl/lib/tpl.ts | Converts implementation to TypeScript and switches to export = for CJS compatibility. |
| packages/tpl/index.ts | Adds TS entrypoint re-exporting the implementation with export =. |
| packages/tpl/index.js | Removes the committed JS entrypoint (now generated by tsc). |
| packages/tpl/.gitignore | Ignores generated build artifacts (.js, .d.ts, maps) in the package folder. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
Comments suppressed due to low confidence (1)
packages/tpl/lib/tpl.ts:10
- The exported
tpltype currently restrictsdatavalues tostring, but in-repo callers pass non-string values (e.g. numeric{max}inpackages/validator/lib/validate.js). This makes the new published typings unnecessarily breaking for TypeScript consumers. Widening the value type and explicitly stringifying replacements keeps runtime behavior the same while improving the public typings.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I requested a review from Copilot because CodeRabbit didn't work (hit a usage limit). |
EvanHahn
force-pushed
the
convert-tpl-to-typescript
branch
from
6a562dd to
d0efb99
Compare
no ref This converts `@tryghost/tpl` to TypeScript, and exports type definitions with the package. This should be backwards-compatible.
EvanHahn
force-pushed
the
convert-tpl-to-typescript
branch
from
d0efb99 to
e8523eb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tpl/lib/tpl.ts`:
- Line 23: The explicit String() call on line 23 in the tpl.ts file changes the
runtime behavior for edge cases like Symbol values, breaking the legacy coercion
semantics. Remove the String() wrapper and instead return the raw value
data[trimmed] directly, allowing the replacer callback to handle the coercion
naturally as it did in the original implementation. This preserves the 1:1
behavior required for this migration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 606ea290-03a2-44e3-84b7-9bf68940494d
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
packages/tpl/.gitignorepackages/tpl/index.jspackages/tpl/index.tspackages/tpl/lib/tpl.tspackages/tpl/package.jsonpackages/tpl/test/tpl.test.tspackages/tpl/tsconfig.jsonpackages/tpl/tsconfig.test.jsonpackages/tpl/vitest.config.ts
💤 Files with no reviewable changes (1)
- packages/tpl/index.js
| throw new ReferenceError(`${trimmed} is not defined`); | ||
| } | ||
| return data[trimmed]; | ||
| return String(data[trimmed]); |
There was a problem hiding this comment.
Preserve legacy interpolation coercion behavior (avoid explicit String() here).
Line 23 introduces a runtime behavior change in a migration that should stay 1:1: explicit String(data[trimmed]) no longer matches legacy replacement semantics for edge cases (e.g., Symbol values). Keep the legacy coercion path by returning the raw value through the replacer callback.
Suggested fix
- return String(data[trimmed]);
+ return data[trimmed] as string;Based on learnings: Wave 1 TS+ESM migrations should preserve runtime behavior and avoid logic changes.
📝 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.
| return String(data[trimmed]); | |
| return data[trimmed] as string; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/tpl/lib/tpl.ts` at line 23, The explicit String() call on line 23 in
the tpl.ts file changes the runtime behavior for edge cases like Symbol values,
breaking the legacy coercion semantics. Remove the String() wrapper and instead
return the raw value data[trimmed] directly, allowing the replacer callback to
handle the coercion naturally as it did in the original implementation. This
preserves the 1:1 behavior required for this migration.
Source: Learnings
no ref
This converts
@tryghost/tplto TypeScript, and exports type definitions with the package. This should be backwards-compatible.