From 156e0d900d01587b2918cc41e1c131a5523673bb Mon Sep 17 00:00:00 2001 From: Sukhada Kulkarni Date: Sat, 21 Mar 2026 13:07:25 -1000 Subject: [PATCH] refactor: remove dead code from src/lib/ Dead code identified via ts-prune + manual grep verification. Deletions: - dst-helper.js + test: unused outside its own test, hand-rolls DST detection that luxon does natively - js-types.ts: misleading types (TruthyString includes falsy values), inlined into the 2 consuming files with better names - __mocks__/ directory: redundant with jest.mock() auto-mocking - getInteractionTree: never imported outside barrel - getDisplayPhoneNumber: never imported outside barrel - getCommonZipRanges: never imported outside barrel - timezonecomplete dependency: only consumer was dst-helper Unexported (internal-only): - getHighestRole: only used by hasRole() within permissions.ts - downloadFromUrl: only used by withTempDownload() within utils.ts Removed unused lodash import from index.ts. Co-Authored-By: Claude Opus 4.6 (1M context) --- __test__/lib/dst-helper.test.js | 85 ------------ docs/ai-docs/codebase-modernization-plan.md | 144 ++++++++++++++++++++ package.json | 1 - src/components/forms/GSDateField.tsx | 3 +- src/components/forms/GSFormField.tsx | 4 +- src/lib/__mocks__/timezones.js | 2 - src/lib/__mocks__/zip-format.js | 2 - src/lib/dst-helper.js | 81 ----------- src/lib/index.ts | 20 +-- src/lib/interaction-step-helpers.js | 10 -- src/lib/js-types.ts | 5 - src/lib/permissions.ts | 2 +- src/lib/phone-format.ts | 6 - src/lib/utils.ts | 2 +- src/lib/zip-format.js | 1 - 15 files changed, 154 insertions(+), 214 deletions(-) delete mode 100644 __test__/lib/dst-helper.test.js create mode 100644 docs/ai-docs/codebase-modernization-plan.md delete mode 100644 src/lib/__mocks__/timezones.js delete mode 100644 src/lib/__mocks__/zip-format.js delete mode 100644 src/lib/dst-helper.js delete mode 100644 src/lib/js-types.ts diff --git a/__test__/lib/dst-helper.test.js b/__test__/lib/dst-helper.test.js deleted file mode 100644 index 7443b1259..000000000 --- a/__test__/lib/dst-helper.test.js +++ /dev/null @@ -1,85 +0,0 @@ -import {DstHelper} from '../../src/lib/dst-helper' -import {DateTime, zone, DateFunctions} from 'timezonecomplete' - -var MockDate = require('mockdate'); - -describe('test DstHelper', () => { - afterEach(() => { - MockDate.reset() - }) - - it('helps us figure out if we\'re in DST in February in New York', () => { - MockDate.set('2018-02-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('America/New_York')) - expect(DstHelper.isOffsetDst(d.offset(), 'America/New_York')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'America/New_York')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'America/New_York')).toBeFalsy() - }) - - it('helps us figure out if we\'re in DST in July in New York', () => { - MockDate.set('2018-07-21T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('America/New_York')) - expect(DstHelper.isOffsetDst(d.offset(), 'America/New_York')).toBeTruthy() - expect(DstHelper.isDateTimeDst(d, 'America/New_York')).toBeTruthy() - expect(DstHelper.isDateDst(new Date(), 'America/New_York')).toBeTruthy() - }) - - it('helps us figure out if we\'re in DST in February in Sydney', () => { - MockDate.set('2018-02-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('Australia/Sydney')) - expect(DstHelper.isOffsetDst(d.offset(), 'Australia/Sydney')).toBeTruthy() - expect(DstHelper.isDateTimeDst(d, 'Australia/Sydney')).toBeTruthy() - expect(DstHelper.isDateDst(new Date(), 'Australia/Sydney')).toBeTruthy() - }) - - it('helps us figure out if we\'re in DST in July in Sydney', () => { - MockDate.set('2018-07-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('Australia/Sydney')) - expect(DstHelper.isOffsetDst(d.offset(), 'Australia/Sydney')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'Australia/Sydney')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'Australia/Sydney')).toBeFalsy() - }) - - it('helps us figure out if we\'re in DST in February in Kathmandu, which has no DST', () => { - MockDate.set('2018-02-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('Asia/Kathmandu')) - expect(DstHelper.isOffsetDst(d.offset(), 'Asia/Kathmandu')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'Asia/Kathmandu')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'Asia/Kathmandu')).toBeFalsy() - }) - - it('helps us figure out if we\'re in DST in July in Kathmandu, which has no DST', () => { - MockDate.set('2018-07-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('Asia/Kathmandu')) - expect(DstHelper.isOffsetDst(d.offset(), 'Asia/Kathmandu')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'Asia/Kathmandu')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'Asia/Kathmandu')).toBeFalsy() - }) - - it('helps us figure out if we\'re in DST in February in Arizona, which has no DST', () => { - MockDate.set('2018-02-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('US/Arizona')) - expect(DstHelper.isOffsetDst(d.offset(), 'US/Arizona')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'US/Arizona')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'US/Arizona')).toBeFalsy() - }) - - it('helps us figure out if we\'re in DST in July in Arizona, which has no DST', () => { - MockDate.set('2018-07-01T15:00:00Z') - let d = new DateTime(new Date(), DateFunctions.Get, zone('US/Arizona')) - expect(DstHelper.isOffsetDst(d.offset(), 'US/Arizona')).toBeFalsy() - expect(DstHelper.isDateTimeDst(d, 'US/Arizona')).toBeFalsy() - expect(DstHelper.isDateDst(new Date(), 'US/Arizona')).toBeFalsy() - }) - - it('correctly reports a timezone\'s offset and whether it has DST', () => { - expect(DstHelper.getTimezoneOffsetHours('America/New_York')).toEqual(-5) - expect(DstHelper.timezoneHasDst('America/New_York')).toBeTruthy() - expect(DstHelper.getTimezoneOffsetHours('US/Arizona')).toEqual(-7) - expect(DstHelper.timezoneHasDst('US/Arizona')).toBeFalsy() - expect(DstHelper.getTimezoneOffsetHours('Europe/Paris')).toEqual(1) - expect(DstHelper.timezoneHasDst('Europe/Paris')).toBeTruthy() - expect(DstHelper.getTimezoneOffsetHours('Europe/London')).toEqual(0) - expect(DstHelper.timezoneHasDst('Europe/London')).toBeTruthy() - }) -}) \ No newline at end of file diff --git a/docs/ai-docs/codebase-modernization-plan.md b/docs/ai-docs/codebase-modernization-plan.md new file mode 100644 index 000000000..abb911a12 --- /dev/null +++ b/docs/ai-docs/codebase-modernization-plan.md @@ -0,0 +1,144 @@ +# Spoke Codebase Modernization Plan + +## Non-Goals + +This plan does **not** cover: +- Feature development or product changes +- Database schema changes or migrations +- Infrastructure, CI/CD pipeline redesign, or deployment changes +- GraphQL schema refactoring +- React component library migration (Material-UI v4 → v5+) +- Full E2E test framework migration (Selenium → Playwright/Cypress) — though we prepare for it +- Performance optimization +- Multi-country support (Spoke is US-only for now; YAGNI applies) + +--- + +## Principles + +1. **Tests first** — add/verify tests before changing anything; every refactor must be provably a no-op +2. **Small, reviewable PRs** — each PR has one theme; stacked PRs where ordering matters +3. **Static analysis for safety** — use `ts-prune`, `knip`, or `eslint-plugin-unused-imports` to find dead code; cross-reference with grep for dynamic imports and GraphQL resolver references +4. **Prefer native/ES2020+ over libraries** — remove dependencies when the language has caught up +5. **Co-located tests** — `.spec.ts` next to source files, not in a separate `__test__/` tree +6. **Incremental coverage requirements** — enforce coverage thresholds on new/changed code + +--- + +## Phase 1: Dead Code Removal + +**Why first:** Smaller codebase = less to convert, test, and maintain. Every file deleted is a file you never have to convert to TypeScript or write tests for. + +**1A. Automated dead code audit** +- Run `knip` or `ts-prune` to identify unused exports, files, and dependencies +- Cross-reference with grep for dynamic imports (`require()`, string-based references) +- Manually verify GraphQL resolvers and Express route handlers (schema-driven, won't show as static imports) +- Each removal gets: `yarn test`, `yarn build`, `yarn lint` + +**1B. Remove dead exports, files, and barrel re-exports** +- Barrel exports (`index.ts` files) that re-export unused functions — change to direct imports, delete barrels +- Unused utility functions, types, and constants +- Orphaned mock files +- `timezonecomplete` dependency (only consumer `dst-helper.ts` already deleted) +- Move `gzip`/`gunzip` (defined inline in `src/lib/index.ts`) to their own file before deleting the barrel + +--- + +## Phase 2: Test Infrastructure + +**Why second:** Everything after this depends on having tests that actually run and catch regressions. + +**2A. Standardize test location** +- Move all `__test__/` tests to co-located `.spec.ts` files next to their source +- ~25 test files in `__test__/` need migration (containers, server, workers) +- Delete `__test__/lib/` (already done for `src/lib/`) +- Remove `__mocks__/` directories; inline mocks in test files using `jest.mock()` + +**2B. Modernize test conventions** +- `it()` + `describe()` consistently (not `test()`) +- Remove `var`, use `const`/`let` +- Jest fake timers instead of `mockdate` where applicable +- Proper test isolation (no shared mutable state between tests) + +**2C. Add coverage requirements** +- Add `--coverage` with thresholds to CI for new/changed files +- Start with a low global threshold (e.g., 30%) and ratchet up +- Require 80%+ coverage for any newly added file +- Configure Jest `coverageThreshold` in `jest.config.js` + +--- + +## Phase 3: TypeScript Conversion + +**Why after dead code removal:** Fewer files to convert. + +**3A. Convert remaining `.js`/`.jsx` files to `.ts`/`.tsx`** +- `src/lib/` already done +- Priority order: `src/server/` → `src/containers/` → `src/components/` +- Each directory gets stricter ESLint rules via `overrides` (same pattern as `src/lib/`) + +**3B. Enable stricter TypeScript rules per-directory** +- `@typescript-eslint/no-explicit-any`: error +- `@typescript-eslint/explicit-module-boundary-types`: error +- Start with `src/lib/` (already done), expand to `src/server/`, then `src/containers/` + +**3C. Replace workaround types** +- Inline misleading types at their usage sites (already done for `js-types.ts` in `src/lib/`) +- Audit for similar patterns in other directories + +--- + +## Phase 4: Library Modernization + +**Why after TS conversion:** TypeScript catches type errors when swapping implementations. + +**4A. Lodash removal** +- **Why:** Lodash was essential pre-ES2015. Most of its functions now have native equivalents. It adds ~70KB to the bundle and creates a false sense that native alternatives don't exist. +- **176 imports across the codebase** — do this directory by directory +- Common replacements: + - `isEmpty(x)` → `!x` or `x.length === 0` or `Object.keys(x).length === 0` + - `isNil(x)` → `x == null` + - `get(obj, 'a.b.c')` → optional chaining `obj?.a?.b?.c` + - `map`, `filter`, `find`, `reduce`, `sortBy`, `reverse` → native array methods + - `flow` → function composition or just sequential calls + - `fromPairs` → `Object.fromEntries` + - `escapeRegExp` — no native equivalent; inline the 1-liner regex + - `isEqual` — no native deep equal; keep lodash or use a smaller library + - `transform` — usually replaceable with `Object.entries().reduce()` +- `lodash/fp` imports (used in `interaction-step-helpers.ts`) already removed for `src/lib/` +- Keep lodash where the native alternative is significantly more complex (deep equal, deep clone) + +**4B. Other library evaluations** +- `humps` → inline `camelize` one-liner or native (only used in `recordToCamelCase`) +- `charset-utils.ts` → evaluate GSM encoding libraries vs hand-rolled implementation +- `zipcode-to-timezone` → replace hand-rolled `commonZipRanges` array (stretch) +- `superagent` → native `fetch` (Node 20+ has global fetch) + +--- + +## Phase 5: Ongoing Code Quality + +Applied incrementally alongside other phases, not as a separate big-bang effort. + +- Replace deprecated timezone names with IANA equivalents +- Prefer explicit function parameters over passing objects (more testable, more readable) +- Consolidate duplicate functions (e.g., two `titleCase` implementations) +- Fix known bugs discovered during testing (document before, fix after) +- `dataTest`: keep for now (Playwright/Cypress also use `data-test` selectors), modernize comment to say "modernize E2E framework" not "remove" + +--- + +## Ordering Rationale + +``` +Phase 1 (Dead Code) → Phase 2 (Tests) → Phase 3 (TypeScript) → Phase 4 (Libraries) + ↑ + Phase 5 (Quality) — continuous alongside all phases +``` + +- Dead code first because deleted files don't need tests, conversion, or library cleanup +- Tests second because everything after needs them as a safety net +- TS before library swaps because the type checker catches errors in replacements +- Quality is continuous — applied within each PR as we touch files + +--- \ No newline at end of file diff --git a/package.json b/package.json index 811d9a6b2..b55b49b42 100644 --- a/package.json +++ b/package.json @@ -178,7 +178,6 @@ "style-loader": "^2.0.0", "superagent": "^4.1.0", "thinky": "^2.3.3", - "timezonecomplete": "^5.11.0", "twilio": "^2.11.0", "tzdata": "^1.0.18", "use-debounce": "^8.0.1", diff --git a/src/components/forms/GSDateField.tsx b/src/components/forms/GSDateField.tsx index 7b987fb15..94a37bd85 100644 --- a/src/components/forms/GSDateField.tsx +++ b/src/components/forms/GSDateField.tsx @@ -3,10 +3,11 @@ import { DatePicker } from "material-ui"; import React from "react"; import { DateTime } from "../../lib/datetime"; -import type { ISODateString } from "../../lib/js-types"; import type { GSFormFieldProps } from "./GSFormField"; import { GSFormField } from "./GSFormField"; +type ISODateString = string | undefined | null | false | 0; + interface GSDateFieldProps { onChange: (d: ISODateString | null) => void; value: string | undefined; diff --git a/src/components/forms/GSFormField.tsx b/src/components/forms/GSFormField.tsx index 8bcf8ff25..10691734f 100644 --- a/src/components/forms/GSFormField.tsx +++ b/src/components/forms/GSFormField.tsx @@ -1,10 +1,10 @@ import { Component } from "react"; import type { FieldProps } from "react-formal/Field"; -import type { TruthyString } from "../../lib/js-types"; +type MaybeString = string | undefined | null | false | 0; export interface GSFormFieldProps extends FieldProps { - floatingLabelText: TruthyString; + floatingLabelText: MaybeString; label: string; ["data-test"]?: any; } diff --git a/src/lib/__mocks__/timezones.js b/src/lib/__mocks__/timezones.js deleted file mode 100644 index d3963efca..000000000 --- a/src/lib/__mocks__/timezones.js +++ /dev/null @@ -1,2 +0,0 @@ -const timezones = jest.genMockFromModule("../timezones"); -module.exports = timezones; diff --git a/src/lib/__mocks__/zip-format.js b/src/lib/__mocks__/zip-format.js deleted file mode 100644 index d8e5a3fca..000000000 --- a/src/lib/__mocks__/zip-format.js +++ /dev/null @@ -1,2 +0,0 @@ -const zipFormat = jest.genMockFromModule("../zip-format"); -module.exports = zipFormat; diff --git a/src/lib/dst-helper.js b/src/lib/dst-helper.js deleted file mode 100644 index 88eee0b1d..000000000 --- a/src/lib/dst-helper.js +++ /dev/null @@ -1,81 +0,0 @@ -/* eslint-disable max-classes-per-file */ -import { DateFunctions, DateTime, zone } from "timezonecomplete"; - -class TimezoneOffsetAndDst { - constructor(tzOffsetMinutes, hasDst) { - this.tzOffsetMinutes = tzOffsetMinutes; - this.hasDst = hasDst; - } -} - -const _timezoneOffsetAndDst = {}; - -// a class to help us know if a date is DST in a given timezone -export class DstHelper { - static ensureTimezoneDstCalculated(timezone) { - if (!(timezone in _timezoneOffsetAndDst)) { - // If a location has DST, the offset from GMT at January 1 and June 1 will certainly - // be different. The greater of the two is the DST offset. For our check, we - // don't care when DST is (March-October in the northern hemisphere, October-March - // in the southern hemisphere). We only care about the offset during DST. - const januaryDate = new DateTime( - new Date().getFullYear(), - 1, - 1, - 0, - 0, - 0, - 0, - zone(timezone) - ); - const julyDate = new DateTime( - new Date().getFullYear(), - 6, - 1, - 0, - 0, - 0, - 0, - zone(timezone) - ); - _timezoneOffsetAndDst[timezone] = new TimezoneOffsetAndDst( - Math.min(januaryDate.offset(), julyDate.offset()), - januaryDate.offset() !== julyDate.offset() - ); - } - } - - static getTimezoneOffsetHours(timezone) { - DstHelper.ensureTimezoneDstCalculated(timezone); - return _timezoneOffsetAndDst[timezone].tzOffsetMinutes / 60; - } - - static timezoneHasDst(timezone) { - DstHelper.ensureTimezoneDstCalculated(timezone); - return _timezoneOffsetAndDst[timezone].hasDst; - } - - static isOffsetDst(offset, timezone) { - DstHelper.ensureTimezoneDstCalculated(timezone); - - // if this timezone has DST (meaning, january and july offsets were different) - // and the offset from GMT passed into this function is the same as the timezone's - // offset from GMT during DST, we return true. - const timezoneOffsetAndDst = _timezoneOffsetAndDst[timezone]; - return ( - timezoneOffsetAndDst.hasDst && - timezoneOffsetAndDst.tzOffsetMinutes + 60 === offset - ); - } - - static isDateDst(date, timezone) { - const d = new DateTime(date, DateFunctions.Get, zone(timezone)); - return DstHelper.isOffsetDst(d.offset(), timezone); - } - - static isDateTimeDst(date, timezone) { - return DstHelper.isOffsetDst(date.offset(), timezone); - } -} - -export default DstHelper; diff --git a/src/lib/index.ts b/src/lib/index.ts index 2b820daf5..ff4f27e23 100644 --- a/src/lib/index.ts +++ b/src/lib/index.ts @@ -1,35 +1,23 @@ -import _ from "lodash"; import zlib from "zlib"; -import { getDisplayPhoneNumber, getFormattedPhoneNumber } from "./phone-format"; +import { getFormattedPhoneNumber } from "./phone-format"; import { sleep } from "./utils"; -export { getFormattedPhoneNumber, getDisplayPhoneNumber }; +export { getFormattedPhoneNumber }; -export { - getFormattedZip, - zipToTimeZone, - getCommonZipRanges -} from "./zip-format"; -export { DstHelper } from "./dst-helper"; +export { getFormattedZip, zipToTimeZone } from "./zip-format"; export { isClient } from "./is-client"; export { sleep }; export { findParent, getInteractionPath, - getInteractionTree, interactionStepForId, getTopMostParent, getChildren, makeTree } from "./interaction-step-helpers"; -export { - ROLE_HIERARCHY, - getHighestRole, - hasRole, - isRoleGreater -} from "./permissions"; +export { ROLE_HIERARCHY, hasRole, isRoleGreater } from "./permissions"; export const gzip = (str: string) => new Promise((resolve, reject) => { diff --git a/src/lib/interaction-step-helpers.js b/src/lib/interaction-step-helpers.js index 11141155e..1e8d70f29 100644 --- a/src/lib/interaction-step-helpers.js +++ b/src/lib/interaction-step-helpers.js @@ -79,16 +79,6 @@ export const getChildren = (interactionStep, allInteractionSteps, isModel) => { return children; }; -export const getInteractionTree = (allInteractionSteps, isModel) => { - const pathLengthHash = {}; - allInteractionSteps.forEach((step) => { - const path = getInteractionPath(step, allInteractionSteps, isModel); - pathLengthHash[path.length] = pathLengthHash[path.length] || []; - pathLengthHash[path.length].push({ interactionStep: step, path }); - }); - return pathLengthHash; -}; - export const getTopMostParent = (interactionSteps, isModel) => sortByNewest(interactionSteps).find((step) => isModel diff --git a/src/lib/js-types.ts b/src/lib/js-types.ts deleted file mode 100644 index 416e1fd64..000000000 --- a/src/lib/js-types.ts +++ /dev/null @@ -1,5 +0,0 @@ -// types for partial porting of js to ts components - -export type Falsy = undefined | null | false | 0; -export type TruthyString = string | Falsy; -export type ISODateString = TruthyString; diff --git a/src/lib/permissions.ts b/src/lib/permissions.ts index 65f9413ab..af8f16e9f 100644 --- a/src/lib/permissions.ts +++ b/src/lib/permissions.ts @@ -19,7 +19,7 @@ export const hasRoleAtLeast = ( wantsRole: UserRoleType ) => ROLE_HIERARCHY.indexOf(hasRole) >= ROLE_HIERARCHY.indexOf(wantsRole); -export const getHighestRole = (roles: UserRoleType[]) => +const getHighestRole = (roles: UserRoleType[]) => roles.sort( (roleA: UserRoleType, roleB: UserRoleType) => ROLE_HIERARCHY.indexOf(roleA) - ROLE_HIERARCHY.indexOf(roleB) diff --git a/src/lib/phone-format.ts b/src/lib/phone-format.ts index d67b0d7d2..ffd0a832d 100644 --- a/src/lib/phone-format.ts +++ b/src/lib/phone-format.ts @@ -17,9 +17,3 @@ export const getFormattedPhoneNumber = (cell: string, country = "US") => { return ""; } }; - -export const getDisplayPhoneNumber = (e164Number: string, country = "US") => { - const phoneUtil = PhoneNumberUtil.getInstance(); - const parsed = phoneUtil.parse(e164Number, country); - return phoneUtil.format(parsed, PhoneNumberFormat.NATIONAL); -}; diff --git a/src/lib/utils.ts b/src/lib/utils.ts index 759d8ea91..37aadafd1 100644 --- a/src/lib/utils.ts +++ b/src/lib/utils.ts @@ -46,7 +46,7 @@ export const difference = ( return changes(object, base); }; -export const downloadFromUrl = async (url: string, filePath: string) => { +const downloadFromUrl = async (url: string, filePath: string) => { let fileDownloaded = false; const file = fs.createWriteStream(filePath); diff --git a/src/lib/zip-format.js b/src/lib/zip-format.js index 4885ad4c2..3ea0f7340 100644 --- a/src/lib/zip-format.js +++ b/src/lib/zip-format.js @@ -82,6 +82,5 @@ const zipToTimeZone = (zip) => { module.exports = { getFormattedZip, - getCommonZipRanges, zipToTimeZone };