Adopt and check TypeScript vis JSDoc - #2735
Merged
Merged
Conversation
The assets are plain JavaScript, so checkJs is off by default and each file opts in with a // @ts-check comment as it gains JSDoc types.
Split the asset tests into their own project so that the browser code isn't checked against Node's globals, and pin strict off now that TypeScript 7 turns it on by default.
Add ambient declarations for the vendored libraries, which stay outside the typecheck.
Declare the $ surface once, as JSDoc typedefs that the implementations are contextually typed by, rather than annotating each helper twice. Typing $.prepend turned up a crash: prepending into an element with no children called $.append(value) without the element to append to, so it read the value as the target and appended undefined.
App's registries are keyed by name and populated by the files that define their members, so they are typed as records rather than enumerating the 37 view classes. Two call sites in onWindowError spread the window.onerror arguments into helpers that read at most the first two; pass those directly instead.
Model copies arbitrary attributes onto itself, so the attributes each subclass relies on are documented rather than declared: a field declaration would run after super() and blank the value out.
The IndexedDB version-mismatch handler used a function expression, so its `this` was the request rather than the DB, and the `error` it reported resolved to the global error template. Make it an arrow and report the request's own error.
DB#unstore's error handler was a function expression, so its `this` was the transaction: the NotFoundError retry threw instead of bumping the schema and trying again. DB#store, right above it, already used an arrow.
Views read their configuration off `this.constructor`, which is typed as a bare Function; route it through a `statics()` accessor instead. Two fixes the typecheck turned up: - Sidebar passed `(newDoc, previousDoc)` to onScopeChange, so the comma operator dropped newDoc and the handler revealed the doc being left instead of the one being entered. - View#findLastByClass indexed the first match rather than the list, so it always returned undefined. It has no callers.
Declare the three base classes at the top level and register them on `app` afterwards, so subclasses extend a type instead of `any`. Models and views stay open-ended through an index signature: their properties come from the manifest and from the `elements` static, and the base class fills the latter in from inside its own constructor, before a subclass's field initializers would run. Three fixes this turned up: - Document#init called addSubview(this.menu, this.addSubview(this.sidebar)), passing the inner call's return value as an argument addSubview ignores. - 19 super.activate/deactivate/empty calls spread `arguments` into methods that take none. - BasePage#deactivate returned the array it had just emptied.
Type the shared helpers and the templates that take arguments; the rest are nullary and already infer as () => string.
The asset tests had no job at all, so they only ever ran locally.
Only the annotations whose value the code never inspects: the ones that stayed any are the widening casts, the two index signatures, and the places that read the value.
The models and collections were class expressions assigned into registries, so they had no nameable type and everything that took one was annotated `any`. Declare them at the top level, as the base classes already are, and make Collection generic over its model so that all(), findBy() and the rest carry the type through. Doc, Entry and Type now replace `any` at 58 parameters and a further 20 returns. The registries are typed by name instead of Record<string, any>. Also corrected while typing: offlineDoc and renderDoc were annotated as taking a map of install statuses when they take one, and offlinePage's first parameter is rendered HTML rather than the docs its name suggests.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Move the models' own properties into globals.d.ts, merged into the classes: Model copies the manifest attributes on, so a field declaration would run after super() and blank them out again. Merging suppresses the inference of this.x =, so the derived properties are declared too. A typo on a doc, entry or type is now an error. Hoist the app services for the same reason as the models: app.settings, app.db and the rest were any because they were class expressions. The service worker keeps its own binding name, since ServiceWorker is a DOM global that a top-level class would shadow. The backup validators now take unknown and return type predicates, so they narrow for their callers rather than just reporting a boolean. Fixes this turned up: three super.match/free/end calls spread arguments into methods taking none; SynchronousSearcher#delay returns nothing where the base returns a timeout handle; InstallStatus#mtime is false rather than undefined when a doc isn't installed; and the import summary's failed list holds slugs, not docs.
$() and the View find helpers return HTMLElement instead of any; the call sites that need a link, a form or a field narrow it themselves.
Settings#get and #set are keyed on a map of what each setting holds, so callers no longer have to narrow the result. Two of the declarations were wrong against the code and are corrected: analyticsConsent is stored as 1 or 0, and spaceTimeout comes back as a string because it isn't an integer.
The searcher takes models rather than untyped rows, DB's shadowed useIndexedDB is written through a view that expects the value, and the event emitter hands listeners unknown arguments — the four listeners that read theirs now say what their event carries. lib/page.js's onclick and onpopstate are renamed: as top-level vars in the concatenated bundle they shadowed the DOM's handler properties of the same name, which is also what forced them to be annotated with the DOM's own handler types.
node --test only searches a directory given as a positional from v25, so CI's Node 24 tried to load test/assets as a module. Pass the files instead, which works on any version. Pin the version in .tool-versions alongside Ruby, so CI and a local checkout agree on one place.
There was a problem hiding this comment.
🟡 Changes recommended
Several ambient preference and document-property types contradict their runtime representations.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds opt-in TypeScript checking through JSDoc across the JavaScript asset pipeline and CI.
Changes:
- Adds TypeScript configuration, ambient declarations, dependencies, and CI checks.
- Annotates models, collections, views, templates, utilities, and tests.
- Refactors global class declarations to support cross-file type references.
File summaries
| File | Description |
|---|---|
tsconfig.json |
Configures browser asset checking. |
test/assets/tsconfig.json |
Configures Node test checking. |
test/assets/search_ranking_test.js |
Enables type checking. |
test/assets/search_hash_test.js |
Adds checking and router typing. |
test/assets/doc_version_test.js |
Enables type checking. |
package.json |
Adds TypeScript and asset scripts. |
assets/javascripts/views/sidebar/type_list.js |
Types the type-list view. |
assets/javascripts/views/sidebar/sidebar.js |
Types the sidebar view. |
assets/javascripts/views/sidebar/sidebar_hover.js |
Types hover behavior. |
assets/javascripts/views/sidebar/results.js |
Types search results. |
assets/javascripts/views/sidebar/entry_list.js |
Types entry lists. |
assets/javascripts/views/sidebar/doc_picker.js |
Types the doc picker. |
assets/javascripts/views/sidebar/doc_list.js |
Types the doc list. |
assets/javascripts/views/search/search.js |
Types search handling. |
assets/javascripts/views/search/search_scope.js |
Types scoped search. |
assets/javascripts/views/pages/support_tables.js |
Types support-table behavior. |
assets/javascripts/views/pages/sqlite.js |
Types SQLite toggles. |
assets/javascripts/views/pages/rdoc.js |
Types RDoc toggles. |
assets/javascripts/views/pages/jquery.js |
Types runnable examples. |
assets/javascripts/views/pages/hidden.js |
Types disabled-doc pages. |
assets/javascripts/views/pages/base.js |
Types the base page view. |
assets/javascripts/views/misc/updates.js |
Types update notifications. |
assets/javascripts/views/misc/tip.js |
Types tip notifications. |
assets/javascripts/views/misc/notif.js |
Types notification infrastructure. |
assets/javascripts/views/misc/notice.js |
Types persistent notices. |
assets/javascripts/views/misc/news.js |
Types news notifications. |
assets/javascripts/views/list/paginated_list.js |
Types list pagination. |
assets/javascripts/views/list/list_select.js |
Types list selection. |
assets/javascripts/views/list/list_fold.js |
Types list folding. |
assets/javascripts/views/list/list_focus.js |
Types keyboard navigation. |
assets/javascripts/views/layout/settings.js |
Types the settings panel. |
assets/javascripts/views/layout/resizer.js |
Types sidebar resizing. |
assets/javascripts/views/layout/path.js |
Types breadcrumbs. |
assets/javascripts/views/layout/mobile.js |
Types mobile layout behavior. |
assets/javascripts/views/layout/menu.js |
Types menu behavior. |
assets/javascripts/views/layout/document.js |
Types the root view. |
assets/javascripts/views/content/type_page.js |
Types type pages. |
assets/javascripts/views/content/static_page.js |
Types static pages. |
assets/javascripts/views/content/settings_page.js |
Types preference handling. |
assets/javascripts/views/content/root_page.js |
Types the root page. |
assets/javascripts/views/content/offline_page.js |
Types offline management. |
assets/javascripts/views/content/entry_page.js |
Types entry loading and caching. |
assets/javascripts/views/content/content.js |
Types content navigation. |
assets/javascripts/tracking.js |
Types analytics bootstrapping. |
assets/javascripts/templates/tip_tmpl.js |
Enables template checking. |
assets/javascripts/templates/sidebar_tmpl.js |
Types sidebar templates. |
assets/javascripts/templates/path_tmpl.js |
Types breadcrumb templates. |
assets/javascripts/templates/pages/type_tmpl.js |
Types type-page templates. |
assets/javascripts/templates/pages/settings_tmpl.js |
Types settings templates. |
assets/javascripts/templates/pages/offline_tmpl.js |
Types offline templates. |
assets/javascripts/templates/pages/help_tmpl.js |
Enables help-template checking. |
assets/javascripts/templates/pages/about_tmpl.js |
Enables about-template checking. |
assets/javascripts/templates/notif_tmpl.js |
Types notification templates. |
assets/javascripts/templates/notice_tmpl.js |
Types notice templates. |
assets/javascripts/templates/error_tmpl.js |
Types error templates. |
assets/javascripts/templates/base.js |
Types generic template rendering. |
assets/javascripts/models/type.js |
Types documentation types. |
assets/javascripts/models/model.js |
Types the base model. |
assets/javascripts/models/entry.js |
Types documentation entries. |
assets/javascripts/models/doc.js |
Types documentation metadata. |
assets/javascripts/lib/page.js |
Types router internals. |
assets/javascripts/lib/local_storage_store.js |
Types local storage access. |
assets/javascripts/lib/license.js |
Enables checking. |
assets/javascripts/lib/favicon.js |
Types favicon handling. |
assets/javascripts/lib/events.js |
Types event emission. |
assets/javascripts/lib/cookies_store.js |
Types cookie storage. |
assets/javascripts/lib/ajax.js |
Types AJAX operations. |
assets/javascripts/globals.d.ts |
Declares shared and vendor globals. |
assets/javascripts/debug.js |
Types debug instrumentation. |
assets/javascripts/collections/types.js |
Types type collections. |
assets/javascripts/collections/entries.js |
Types entry collections. |
assets/javascripts/collections/docs.js |
Types doc collections. |
assets/javascripts/collections/collection.js |
Adds generic collection typing. |
assets/javascripts/application.js |
Enables bundle checking. |
assets/javascripts/app/update_checker.js |
Types update checks. |
assets/javascripts/app/shortcuts.js |
Types keyboard shortcuts. |
assets/javascripts/app/settings.js |
Types stored preferences. |
assets/javascripts/app/serviceworker.js |
Types service-worker handling. |
assets/javascripts/app/searcher.js |
Types search implementation. |
assets/javascripts/app/router.js |
Types application routing. |
assets/javascripts/app/offline_backup.js |
Types backup import/export. |
.gitignore |
Ignores installed Node modules. |
.github/workflows/test.yml |
Adds asset checks to CI. |
Review details
- Files reviewed: 85/89 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+10
to
+26
| * @property {boolean} hideDisabled | ||
| * @property {boolean} hideIntro | ||
| * @property {number} news When the changelog was last read, as a Unix timestamp. | ||
| * @property {boolean} manualUpdate | ||
| * @property {number} schema The offline database's schema version. | ||
| * @property {boolean | number} analyticsConsent Stored as 1 or 0. | ||
| * @property {string} theme `"auto"`, `"dark"` or `"default"`. | ||
| * @property {number} spaceScroll How far space scrolls, as a fraction of the viewport. | ||
| * @property {number | string} spaceTimeout How long after typing space stops | ||
| * scrolling, in seconds. Not an integer, so it comes back as a string. | ||
| * @property {boolean} noDocSpecificIcon | ||
| * @property {boolean} autoLatestVersion | ||
| * @property {number} version The build the user last saw. | ||
| * @property {boolean} fastScroll | ||
| * @property {boolean} arrowScroll | ||
| * @property {boolean} noAutofocus | ||
| * @property {boolean} autoInstall |
CookiesStore writes true as 1 and parses the digit back out on read, so a flag comes back as a number rather than the boolean it went in as; a flag that was never written falls back to its default, which is a real boolean. Declaring these as boolean promised callers a strictness the store doesn't provide. Both accessors that passed one straight out are predicates used only for their truthiness, so they now coerce rather than hand back the number. Also correct alias, which the manifest always emits and sets to null. Reported by Copilot on #2735.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.