Skip to content

feat: convert TodoMVC to TypeScript - #6

Open
akhileshthite wants to merge 3 commits into
mainfrom
feat/ts-conversion
Open

akhileshthite wants to merge 3 commits into
mainfrom
feat/ts-conversion

Conversation

@akhileshthite

Copy link
Copy Markdown
Member

Closes #2.

Converts the app, the tests and the scripts to TypeScript, typed against the
declarations @chelonia/lib already ships. npm run typecheck checks it, and
CI runs it.

It is a straight conversion. Stripped of their types, the files do what the
JavaScript did, apart from a few guards the stricter checks asked for where a
missing value would have thrown on undefined anyway.

  • The two contracts stay JavaScript. chel signs the file as it is and Chelonia
    runs that same text, so there is no step that could strip types.
  • Node runs the scripts and tests by stripping the types itself, which needs
    Node 22.18, so engines and the README say so.

Testing: typecheck, lint, 14 unit and 35 end to end tests pass.

AI Usage: Claude Opus 5.5 Max
Self review: Approved

@akhileshthite akhileshthite self-assigned this Sep 30, 2026
@akhileshthite akhileshthite added documentation Improvements or additions to documentation enhancement New feature or request labels Sep 30, 2026
@socket-security

socket-security Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Updated@​chelonia/​cli@​3.4.0 ⏵ 3.4.171 +210099 +189 +170
Updated@​chelonia/​lib@​1.5.0 ⏵ 2.0.073 +110090 +19170
Addedvue-tsc@​2.2.01001007596100
Addedtypescript-eslint@​8.19.11001007598100
Added@​types/​node@​22.10.51001008196100
Addedtypescript@​5.7.31001009010090

View full report

import path from 'node:path'
import process from 'node:process'
import { chel } from './chel.mjs'
import { chel } from './chel.ts'

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This isn't wrong per se, but can we make a decision as to which convention we follow for TS imports (this isn't specific to this PR / repository)?

We have three main options:

  1. Follow the Node + TS convention and use .js imports for .ts files (Deno calls this sloppy imports)
  2. Follow the Deno convention and use .ts imports for .ts files
  3. Follow Node conventions for Node projects and Deno conventions for Deno projects

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yep, I'd go with 2. Node's own type stripping, which runs our scripts and tests here, only finds .ts (a .js import fails with ERR_MODULE_NOT_FOUND), and Deno wants .ts too.

Comment thread src/chelonia/todos.ts Outdated
if (!reducer) return todos
const next = reducer(...w.args)(todos)
return next === KV_NOOP ? todos : next
const next = reducer(...w.args as never[])(todos)

@corrideat corrideat Oct 2, 2026 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why is this using never[]? It looks like a code smell to get around restrictions on any.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The args come back from JSON, so they can't really be typed. It's an explicit any now with a comment, and the casts are gone.

Comment thread src/components/AccountPanel.vue Outdated
import { AuthError, changePassword, deleteAccount } from '../chelonia/index.ts'

const emit = defineEmits(['close'])
const emit = defineEmits<{ close: [] }>()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is not just a type change.

@taoeffect

taoeffect commented Oct 2, 2026 •

Copy link
Copy Markdown
Member

/review


AI review started.

@github-actions

github-actions Bot commented Oct 2, 2026 •

Copy link
Copy Markdown

Advanced AI Review

  • Type: Agentic (crush)
  • Model: glm-5.3
Click to expand review

Code Review

Base: origin/main (a82c5e5)
Head: 6ba1bfe (detached HEAD, "feat: convert TodoMVC to TypeScript")
Date: 2026-10-02
Model: glm-5.3


1. ⚪ Misaligned indentation in addToLists

  • Addressed
  • Dismissed

When the arrow body was wrapped onto its own line, the first property kept its
old 2-space indent while its siblings moved to 4 spaces, leaving one crooked
line in an otherwise consistent file:

src/chelonia/lists.ts:212-217

const addToLists = (identityContractID: ContractID, contractID: ContractID): Promise<unknown> =>
  sbp('chelonia/kv/update', {
  contractID: identityContractID,
    key: LISTS_KEY,
    updater: addList(contractID)
  })

contractID: identityContractID, should be indented 4 spaces to match key:
and updater: (the neighbouring renameList, reformatted in the same commit,
is aligned correctly). StandardJS has no indent rule, so lint does not catch
this. Cosmetic only.

2. ⚪ Unsound as Error cast for the boot error

  • Addressed
  • Dismissed

src/main.ts:8-14 types bootError as Error | null and passes it to App
as bootError?: Error | null, but the rejection value is cast rather than
checked:

let bootError: Error | null = null

startChelonia()
  .catch((e: unknown) => {
    bootError = e as Error

Chelonia failures surface through sbp, which can reject with non-Error values;
if that ever happens, a string (say) flows into a prop typed Error. Today
App.vue only tests the prop for truthiness (v-if="bootError"), so nothing
breaks, but the type is a lie and any future .message/.stack read on it
would silently misbehave. Normalizing instead of casting keeps the new types
honest:

bootError = e instanceof Error ? e : new Error(String(e))

3. ⚪ Redundant union in addTodo's parameter type

  • Addressed
  • Dismissed

src/chelonia/todos-model.ts:43-45:

export const addTodo = (
  { id, title, createdDate }: SortedTodo | { id: string, title: string, createdDate: string }
): Reducer<Todos> => (prev) => ({

SortedTodo is Todo & { id: string }, i.e. the plain object type plus a
completed field, and completed is ignored here (the reducer always writes
completed: false). SortedTodo is therefore structurally a subtype of the
second union member, so the union accepts exactly the same values as the plain
object type alone. The single object type says the same thing more clearly:

export const addTodo = (
  { id, title, createdDate }: { id: string, title: string, createdDate: string }
): Reducer<Todos> => (prev) => ({

No correctness, security or behavioral issues were found. The conversion is
faithful: the few behavior deltas are guards the stricter checks asked for
(changePassword's keep() throwing a clear AuthError on a missing key,
ListsBar.share()'s early return, the contractState() helper replacing
silent undefined dereferences), and each is an improvement over the
TypeError the old code would have thrown on the same broken state. The
queue/JSON dispatch casts (never[], as [string, ...unknown[]]) are
confined to the inherently untyped replay path. Library type imports
(ChelContractState, ChelRootState, KvMirrorEntry, KvNoop) were
verified against the pinned @chelonia/lib 1.5.0 declarations, and no stale
.js/.mjs references remain in code or docs.


Review generated using glm-5.3 via Z.AI. Comment /review to re-run.

* chore: move to @chelonia/lib 2.0.0 and chel 3.4.1

* fix: say a taken username is taken at publish too, drop the old-accounts note
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Convert to TypeScript

3 participants