Convert codebase to TypeScript#24
Conversation
|
My man, this is a herculean effort, it might take me a while to review, though I do want to see it merged. There have also been some changes, so if you don't mind rebasing it, I would appreciate it a lot. I might end up merging #23 first, but I will come back to this! |
|
will need to refresh my memory. but will do @hellcp |
There was a problem hiding this comment.
Pull request overview
This PR converts the Clay codebase (runtime, config-page bundle, and Karma/Mocha tests) from JavaScript to TypeScript, introducing a TS build/typecheck pipeline and a set of ambient typings for Browserify-resolved assets and legacy dependencies.
Changes:
- Replaces core runtime modules (Clay, ClayConfig, ClayItem, ClayEvents, manipulators, components) with TypeScript equivalents.
- Updates build/test tooling to compile TS via
tsify(Gulp + Karma) and addstsconfigfiles for typecheck vs declaration emission. - Reworks test suite to TypeScript and introduces/updates ambient
.d.tsshims for Browserify assets and untyped dependencies.
Reviewed changes
Copilot reviewed 59 out of 68 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| types/tosource.d.ts | Adds ambient typings for tosource. |
| types/joi.d.ts | Adds minimal ambient typings for joi used in tests. |
| types/deepcopy.d.ts | Adds ambient typings for the minified deepcopy entrypoint. |
| types/config-page-html.d.ts | Removes old, narrowly-scoped HTML module typing. |
| types/browserify.d.ts | Adds ambient typings for Browserify-loaded *.tpl, *.css, and config-page HTML. |
| tsconfig.typecheck.json | Removes dedicated typecheck config in favor of main tsconfig.json. |
| tsconfig.json | Adds new primary TS config for strict typechecking (noEmit). |
| tsconfig.declarations.json | Removes old declarations-only config aimed at JS sources. |
| tsconfig.build.json | Adds declarations-only build config targeting dist-types/. |
| test/type-checks.ts | Removes the prior curated type-surface validation test. |
| test/test-utils.ts | Ports test helpers to TS. |
| test/test-utils.js | Removes JS version of test helpers. |
| test/spec/lib/utils.ts | Ports utils unit tests to TS. |
| test/spec/lib/utils.js | Removes JS utils tests. |
| test/spec/lib/manipulators.ts | Ports manipulators tests to TS with additional typing. |
| test/spec/lib/clay-item.ts | Ports ClayItem tests to TS. |
| test/spec/lib/clay-item.js | Removes JS ClayItem tests. |
| test/spec/lib/clay-events.ts | Ports ClayEvents tests to TS. |
| test/spec/lib/clay-config.ts | Ports ClayConfig tests to TS. |
| test/spec/index.ts | Ports main Clay API tests to TS and updates stubbing strategy. |
| test/spec/components/slider.ts | Ports slider component tests to TS. |
| test/spec/components/slider.js | Removes JS slider tests. |
| test/spec/components/select.ts | Ports select component tests to TS. |
| test/spec/components/select.js | Removes JS select tests. |
| test/spec/components/index.ts | Ports components “shape” tests to TS (Joi-based schema). |
| test/spec/components/index.js | Removes JS components index tests. |
| test/spec/components/color.ts | Ports color component tests to TS. |
| test/karma.conf.js | Updates Karma Browserify pipeline to compile TS (tsify) and adjusts coverage thresholds. |
| test/fixture.ts | Ports fixture helpers to TS. |
| test/fixture.js | Removes JS fixture helpers. |
| src/scripts/vendor/minified.d.ts | Adds a single TS declaration source for minified.js globals and module export. |
| src/scripts/lib/utils.ts | Ports utils lib to TS and exports via export =. |
| src/scripts/lib/types.ts | Introduces shared TS interfaces for Clay internals/public structures. |
| src/scripts/lib/manipulators.ts | Ports built-in manipulators to TS. |
| src/scripts/lib/manipulators.js | Removes JS manipulators. |
| src/scripts/lib/component-registry.ts | Ports component registry to TS with explicit typing. |
| src/scripts/lib/component-registry.js | Removes JS component registry. |
| src/scripts/lib/clay-item.ts | Ports ClayItem to TS (factory constructor pattern). |
| src/scripts/lib/clay-item.js | Removes JS ClayItem implementation. |
| src/scripts/lib/clay-events.ts | Ports ClayEvents mixin to TS. |
| src/scripts/lib/clay-events.js | Removes JS ClayEvents implementation. |
| src/scripts/lib/clay-config.ts | Ports ClayConfig to TS (factory constructor pattern + static registerComponent). |
| src/scripts/lib/clay-config.js | Removes JS ClayConfig implementation. |
| src/scripts/config-page.ts | Ports config-page entry to TS. |
| src/scripts/config-page.js | Removes JS config-page entry. |
| src/scripts/components/toggle.ts | Converts component module export style to TS export =. |
| src/scripts/components/text.ts | Converts component module export style to TS export =. |
| src/scripts/components/submit.ts | Converts component module export style to TS export =. |
| src/scripts/components/slider.ts | Ports slider component to TS. |
| src/scripts/components/slider.js | Removes JS slider component. |
| src/scripts/components/select.ts | Ports select component to TS. |
| src/scripts/components/select.js | Removes JS select component. |
| src/scripts/components/radiogroup.ts | Converts component module export style to TS export =. |
| src/scripts/components/input.ts | Converts component module export style to TS export =. |
| src/scripts/components/index.ts | Ports components index to TS with typed component registry. |
| src/scripts/components/index.js | Removes JS components index. |
| src/scripts/components/heading.ts | Converts component module export style to TS export =. |
| src/scripts/components/footer.ts | Converts component module export style to TS export =. |
| src/scripts/components/color.ts | Ports color component to TS with added runtime guards. |
| src/scripts/components/checkboxgroup.ts | Converts component module export style to TS export =. |
| src/scripts/components/button.ts | Converts component module export style to TS export =. |
| package.json | Updates entrypoints/types path, scripts, and adds TS-related dev deps. |
| package-lock.json | Locks new TS-related dependencies and updates package version fields. |
| index.ts | Replaces root JS entry with TS implementation of Clay API. |
| index.js | Removes root JS entry. |
| index.d.ts | Removes curated top-level type declarations. |
| gulpfile.js | Updates build pipeline to bundle TS via tsify and changes typecheck task. |
| .npmignore | Adjusts published artifacts to include dist-types/ instead of root index.d.ts. |
Comments suppressed due to low confidence (3)
src/scripts/components/color.ts:200
- When config.layout is a string, layout is assigned via standardLayouts[configLayout] without validating the key exists. If an unknown layout string is provided, layout becomes undefined and later flattenLayout/layout indexing will throw at runtime. Add a fallback (e.g., only override when standardLayouts[configLayout] is defined, otherwise keep autoLayout()).
test/spec/index.ts:635 - This test claims to validate behavior for sparse arrays, but it now uses an object literal typed as Record<number, string> instead of an actual Array with holes. This changes the runtime code path (object vs Array) and weakens coverage of the sparse-array handling. Use an actual array (e.g., const sparseArr: (string | undefined)[] = []; sparseArr[1] = 'two'; ...) so Clay.prepareForAppMessage is exercised in its Array branch.
test/spec/components/color.ts:333 - In the "only shows the picker if the item is enabled" test, pickerWrap is an HTMLElement (from pickerElements[0]), but the code then reads pickerWrapRecord.get(...) as if it were a Minified M wrapper. HTMLElement doesn't have a .get method, so this will throw at runtime. Use pickerWrap.classList.contains('show') (or keep the M wrapper and use $picker[0].classList...) for the assertions.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 59 out of 68 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/spec/lib/manipulators.ts:51
- Using optional chaining when calling
disable()(and similarlyenable()/hide()/show()below) weakens this test: if the manipulator method is accidentally not attached, the test will silently pass instead of failing. Since these tests are meant to verify those methods exist and work, call them directly (or assert their presence first) so regressions are caught.
|
@hellcp i've reworked the changes to retain the file history continuity, and hopefully made it a bit more readable/approachable |
- Add tsconfig.json and tsconfig.build.json - Add TypeScript and tsify dependencies - Update gulpfile for TypeScript compilation - Add vendor type declarations (minified, browserify, deepcopy, joi, tosource) - Add shared type definitions (src/scripts/lib/types.ts) - Update karma.conf.js: tsify plugin config, MIME type for .ts files, coverage thresholds adjusted for Istanbul/TS source-map artifacts
Pure renames with no content changes to preserve git history lineage. - src/scripts/lib/utils.js → utils.ts - src/scripts/lib/component-registry.js → component-registry.ts - src/scripts/lib/clay-events.js → clay-events.ts - src/scripts/lib/manipulators.js → manipulators.ts - src/scripts/lib/clay-item.js → clay-item.ts - src/scripts/lib/clay-config.js → clay-config.ts
- utils: type annotations, includesCapability null guard, istanbul ignore for source-map artifact on export boundary - component-registry: type annotations for component storage - clay-events: typed event emitter with generic handlers - manipulators: typed manipulator definitions, normalizeColorValue() to preserve string colour values, istanbul ignores for ternary/property artifacts - clay-item: typed ClayItem with component lifecycle, istanbul ignore for initialize function guard - clay-config: typed config builder, remove redundant activeWatchInfo null guard (handled by includesCapability)
Pure renames with no content changes to preserve git history lineage. - button, checkboxgroup, color, footer, heading, index, input, radiogroup, select, slider, submit, text, toggle
- Add type annotations to all 13 component modules - color: full TypeScript conversion with typed layouts, sunlight color map, LAB colour distance functions, istanbul ignores for grid rendering ternary artifacts - select: typed option rendering - slider: typed value display with precision handling, istanbul ignore for property access artifact - index: typed component registry with module imports
Pure renames with no content changes to preserve git history lineage. - index.js → index.ts - src/scripts/config-page.js → config-page.ts
- index.ts: full type safety with ClayInstance interface, typed Pebble API, proper tosource import via require, istanbul ignores for TS defensive guard branches - config-page.ts: typed DOM manipulation and script injection
Pure renames with no content changes to preserve git history lineage. - fixture, test-utils, and all spec files
- Add type annotations to all test fixtures and specs - fixture: typed test helpers with proper ClayConfig types - color tests: simplified assertions matching original JS style - index tests: proper tosource import via require - All test utilities typed for strict mode
- Remove curated index.d.ts (superseded by TypeScript source) - Remove transitional tsconfig.declarations.json and tsconfig.typecheck.json - Remove test/type-checks.ts (no longer needed) - Remove types/config-page-html.d.ts (consolidated into vendor types)
- Replace import X = require('Y') with import X from 'Y'
- Replace export = X with export default X
- Remove 'use strict' directives (ESM is strict by default)
- Keep runtime require() calls for browserify transforms:
template/style require() for stringify transform,
package.json require() for version extraction,
message_keys require() for browserify stub
Full TypeScript conversion of all source, components, entry points, and tests.
If accepted, would supersede #23.