Repository navigation
feat: convert TodoMVC to TypeScript - #6
akhileshthite wants to merge 3 commits into
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
| import path from 'node:path' | ||
| import process from 'node:process' | ||
| import { chel } from './chel.mjs' | ||
| import { chel } from './chel.ts' |
There was a problem hiding this comment.
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:
- Follow the Node + TS convention and use
.jsimports for.tsfiles (Deno calls this sloppy imports) - Follow the Deno convention and use
.tsimports for.tsfiles - Follow Node conventions for Node projects and Deno conventions for Deno projects
There was a problem hiding this comment.
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.
| if (!reducer) return todos | ||
| const next = reducer(...w.args)(todos) | ||
| return next === KV_NOOP ? todos : next | ||
| const next = reducer(...w.args as never[])(todos) |
There was a problem hiding this comment.
Why is this using never[]? It looks like a code smell to get around restrictions on any.
There was a problem hiding this comment.
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.
| import { AuthError, changePassword, deleteAccount } from '../chelonia/index.ts' | ||
|
|
||
| const emit = defineEmits(['close']) | ||
| const emit = defineEmits<{ close: [] }>() |
There was a problem hiding this comment.
This is not just a type change.
|
/review AI review started. |
Advanced AI Review
Click to expand reviewCode ReviewBase: 1. ⚪ Misaligned indentation in
|
* 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
Closes #2.
Converts the app, the tests and the scripts to TypeScript, typed against the
declarations
@chelonia/libalready ships.npm run typecheckchecks it, andCI 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
undefinedanyway.runs that same text, so there is no step that could strip types.
Node 22.18, so
enginesand 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