diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4e59f80..59dfa2f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -195,6 +195,10 @@ jobs: with: playwright: true exitOnceUploaded: true + # A build on the baseline branch does not accept its own snapshots. + # Without this they stay unreviewed, and a later pull request finds no + # accepted ancestor to compare against, so it re-reports every one. + autoAcceptChanges: main # A job rather than its own workflow, because `needs` is what makes the publish # wait on the gate. The write permissions sit here so the job running untrusted diff --git a/.stylelintrc.json b/.stylelintrc.json index 3007718..4be2792 100644 --- a/.stylelintrc.json +++ b/.stylelintrc.json @@ -7,6 +7,118 @@ { "message": "Class selectors are camelCase, so a component reads them back as styles.thisName rather than through a bracketed string" } + ], + "property-disallowed-list": [ + [ + "left", + "right", + "margin-left", + "margin-right", + "padding-left", + "padding-right", + "border-left", + "border-right", + "border-left-color", + "border-left-style", + "border-left-width", + "border-right-color", + "border-right-style", + "border-right-width" + ], + { + "message": "Inline-axis geometry is logical, so one stylesheet serves both reading directions: use inset-inline-start, margin-inline-end, padding-inline or border-inline-end" + } + ], + "declaration-property-value-disallowed-list": [ + { + "text-align": ["/^(?:left|right)$/"], + "float": ["/^(?:left|right)$/"], + "clear": ["/^(?:left|right)$/"], + "outline": [ + "/(? { - // Seeded from the address so the first render already asks for the right - // rows. Without this the effect below issues a request for the empty term and - // then immediately another for the restored one, so every shared link costs - // two requests on a cold start; the stale-result guard makes that survivable - // rather than correct. - // - // This is a read of the address and stays one. The single write lives with - // the table's view state, one layer down, and adding a second writer here is - // what would make the address a thing two components argue over. + // Seeded from the address, or every shared link costs two requests on a cold + // start. A read and it stays one: the single write is one layer down. const [searchTerm, setSearchTerm] = useState(() => parseSearchTerm(window.location.search), ); @@ -26,11 +19,8 @@ const App = () => { const { catalog, tag } = useLocale(); useEffect(() => { - // The asynchronous work sits directly in the effect rather than behind a - // memoized callback, because that is what lets this one variable guard - // every state write below, the settle handler included. A result that - // arrives after the cleanup has run belongs to a search the user has - // already moved past, so it is dropped rather than rendered. + // The fetch sits in the effect so this one flag guards every write below. + // A result arriving after cleanup belongs to a search already moved past. let ignore = false; // Clear the last failure as the new attempt starts, so a retry does not @@ -47,10 +37,8 @@ const App = () => { if (err instanceof Error) { dispatch({ type: "failed", error: err }); } else { - // A rejection carrying no error at all, which the loader never - // produces and a stubbed seam can. It enters state as a dataset - // error so what the reducer holds is always something the translator - // below has a sentence for. + // A rejection carrying no error, which only a stubbed seam produces. + // It enters state as a dataset error so a sentence always exists. dispatch({ type: "failed", error: new DatasetError( @@ -71,29 +59,20 @@ const App = () => { }; }, [searchTerm, retryAttempt]); - // Receives the term the search box has settled on rather than every - // keystroke: the debounce lives with the box now, so what arrives here is - // already the term worth issuing a request for. Memoized because the child - // holds on to it, and an empty dependency array is what makes that hold safe. + // Receives the settled term rather than every keystroke. Memoized with an + // empty dependency array, because the child holds on to it. const handleSearchChange = useCallback((term: string) => { setSearchTerm(term); }, []); - // The next keystroke would re-run the search too, but nothing on screen says - // so, which leaves a reader of the error with no way forward. Retrying on a - // timer with backoff was rejected instead: it hides a misconfigured - // deployment behind a spinner and re-downloads several megabytes of city data - // with nobody watching. + // Deliberately manual. A backoff timer would hide a misconfigured deployment + // behind a spinner and re-download the dataset with nobody watching. const handleRetry = useCallback(() => { dispatch({ type: "retry" }); }, []); - // Derived here, during render, rather than at the catch above, and that is - // the whole reason this line is not two lines further up. The catch is inside - // the fetch effect, so reading the catalog there would make the locale a - // dependency of the effect and a reader changing language would re-issue the - // search. Here the catalog is already in scope and the effect's dependencies - // are untouched. + // Derived during render rather than at the catch, which is inside the fetch + // effect: reading the catalog there would make the locale a dependency of it. const errorMessage = error === null ? null : datasetErrorText(error, catalog, tag); diff --git a/src/api/getCities.test.ts b/src/api/getCities.test.ts index d08b58c..3c8503e 100644 --- a/src/api/getCities.test.ts +++ b/src/api/getCities.test.ts @@ -1,5 +1,6 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import { SEARCH_KEY_SEPARATOR } from "../data/worldcities/cities"; import { getCities } from "./getCities"; import type { City } from "./getCities"; import { @@ -172,6 +173,31 @@ describe("getCities", () => { expect(await getCities({ searchTerm: "admin" })).toHaveLength(0); }); + it("matches nothing for a term carrying the field separator", async () => { + // The address is an input path the box is not: ?q=%00 decodes to a real + // separator and trim leaves it, so a term could otherwise span the join. + // No field's content holds a separator, so such a term matches nothing. + const spanning = await getCities({ + searchTerm: `Tokyo${SEARCH_KEY_SEPARATOR}Tokyo`, + }); + expect(spanning).toHaveLength(0); + + // Not rewritten into a search for the term without it. + expect(await getCities({ searchTerm: "TokyoTokyo" })).toHaveLength(0); + const embedded = await getCities({ + searchTerm: `To${SEARCH_KEY_SEPARATOR}kyo`, + }); + expect(embedded).toHaveLength(0); + expect((await getCities({ searchTerm: "Tokyo" })).length).toBeGreaterThan( + 0, + ); + + // A term of nothing but a separator answers nothing, not every row. + const bare = await getCities({ searchTerm: SEARCH_KEY_SEPARATOR }); + expect(bare).toHaveLength(0); + expect((await getCities({ searchTerm: "" })).length).toBeGreaterThan(0); + }); + it("returns an empty list when nothing matches", async () => { const result = await getCities({ searchTerm: "zzzzzzzz" }); expect(result).toHaveLength(0); diff --git a/src/api/getCities.ts b/src/api/getCities.ts index 61a8f78..41f7d94 100644 --- a/src/api/getCities.ts +++ b/src/api/getCities.ts @@ -1,14 +1,11 @@ import type { City } from "../data/worldcities/cities"; -import { loadCities } from "../data/worldcities/cities"; +import { SEARCH_KEY_SEPARATOR, loadCities } from "../data/worldcities/cities"; -// Re-exported so no consumer's import path changes: the definition moved, the -// import site did not. +// Re-exported so no consumer's import path changed when the definition moved. export type { City }; -// The failure vocabulary reaches the rest of the tree through this seam too, so -// the loader keeps exactly one consumer. The application layer and the catalogs -// need the code to choose a sentence; neither has any business reaching past -// this module to get it. +// The failure vocabulary reaches the tree through this seam too, so the loader +// keeps exactly one consumer. export type { DatasetErrorCode } from "../data/worldcities/cities"; export { DATASET_ERROR_CODES, DatasetError } from "../data/worldcities/cities"; @@ -16,35 +13,35 @@ export interface GetCitiesParams { searchTerm?: string; } -/** - * Simulated network latency, in milliseconds. - */ +/** Simulated network latency, in milliseconds. */ const LATENCY_MS = 200; -/** - * Fake API that returns cities matching a search term against city name, ascii - * name, country name, or country code. Capital is rendered by the table but is - * not matched, because its values are classification codes rather than anything - * a reader searches for; the loader's key comment carries the reasoning. The - * dataset itself is downloaded once and cached, so a failed download is what - * rejects here. - */ +/** Matches a term against name, ascii name, country and country code. */ export async function getCities({ searchTerm = "", }: GetCitiesParams = {}): Promise { const all = await loadCities(); const needle = searchTerm.trim().toLowerCase(); - // The empty term returns a copy, so the module-scope cache cannot escape - // and both branches hand back an array the caller owns. Copying 50,250 - // references costs less than the latency below. - const matched = needle - ? all.filter((city) => city.searchKey.includes(needle)) - : [...all]; - - // The latency is applied to the filter rather than only to the download, so a - // cache-warm call still behaves like a network call and the container's - // debounce timing keeps its meaning. + + // Answered here rather than at the URL parser, so a term arriving any other + // way is answered the same. The separator marks a field boundary in the + // search key, so no field's content holds one and a term carrying one matches + // nothing. Deleting it instead would answer a different search, and a term of + // nothing but a separator would answer every row. + // + // The empty term returns a copy, so the module-scope cache cannot escape. + let matched: City[]; + if (needle === "") { + matched = [...all]; + } else if (needle.includes(SEARCH_KEY_SEPARATOR)) { + matched = []; + } else { + matched = all.filter((city) => city.searchKey.includes(needle)); + } + + // Applied to the filter as well as the download, so a cache-warm call still + // behaves like a network call and the debounce timing keeps its meaning. await new Promise((resolve) => { setTimeout(resolve, LATENCY_MS); }); diff --git a/src/appState.ts b/src/appState.ts index 2ae8cd0..3ddb979 100644 --- a/src/appState.ts +++ b/src/appState.ts @@ -1,41 +1,23 @@ import type { City } from "./api/getCities"; /** - * Everything the container remembers about the collection it is fetching, in - * one object. - * - * It is one object rather than five values because the fetch effect moves - * several of them together: an attempt raises the loading flag and clears the - * previous failure at the same instant, and separate writes are separate - * chances to tear. + * Everything the container remembers about the collection it is fetching. One + * object, because the effect moves several of these together. */ export interface AppState { readonly cities: City[]; readonly error: Error | null; readonly loading: boolean; - /** - * The loading flag is also true for a refetch that follows an empty result - * set, so on its own it cannot say whether the wait is a download or a - * search. This records the one fact it cannot carry: whether the collection - * has arrived at least once. - */ + /** Whether the collection has arrived once, which loading cannot say. */ readonly datasetReady: boolean; - /** - * The retry control increments this, and the effect lists it as a dependency, - * which is what lets a failed load be run again without a page reload. - */ + /** Incremented by the retry control and listed as an effect dependency. */ readonly retryAttempt: number; } /** - * The five things that happen to a request, named for what happened rather than - * for the fields they write. - * - * Settling is its own action rather than folded into the two outcomes before - * it, because clearing the loading flag from one handler that runs whichever - * way the promise went is precisely what stops a failure leaving a permanent - * spinner. Folding it in would leave that guarantee resting on two branches - * agreeing forever. + * The five things that happen to a request. Settling is its own action, because + * folding it in would leave the no-permanent-spinner guarantee resting on two + * branches agreeing forever. */ export type AppAction = | { readonly type: "attempt" } @@ -44,12 +26,7 @@ export type AppAction = | { readonly type: "settled" } | { readonly type: "retry" }; -/** - * Where the container starts: nothing fetched, nothing failed, nothing in - * flight, and no attempt made. The first attempt is what raises the loading - * flag, so the value here is the state of a container that has just mounted and - * not yet run its effect. - */ +/** Where the container starts: mounted, with its effect not yet run. */ export const INITIAL_APP_STATE: AppState = { cities: [], error: null, @@ -58,12 +35,7 @@ export const INITIAL_APP_STATE: AppState = { retryAttempt: 0, }; -/** - * The only thing that moves the container from one request state to the next. - * - * It is pure and knows nothing about React, so the rules below can be read and - * tested without rendering anything. - */ +/** The only thing that moves the container from one request state to the next. */ export function applyAppAction(state: AppState, action: AppAction): AppState { switch (action.type) { case "attempt": @@ -78,9 +50,8 @@ export function applyAppAction(state: AppState, action: AppAction): AppState { datasetReady: true, }; case "failed": - // The rows already on screen are correct until something replaces them, - // so a failure paints an error beside them rather than emptying the - // table, and the arrival flag stays raised. + // The rows on screen stay correct until something replaces them, so a + // failure paints beside them rather than emptying the table. return { ...state, error: action.error }; case "settled": return { ...state, loading: false }; diff --git a/src/components/DataTable/DataTable.tsx b/src/components/DataTable/DataTable.tsx index e9e9a3a..0dd893b 100644 --- a/src/components/DataTable/DataTable.tsx +++ b/src/components/DataTable/DataTable.tsx @@ -10,23 +10,7 @@ import styles from "./DataTable.module.scss"; export type { PaginationLabels }; -/** - * Every string this table and the controls under it render. - * - * They arrive as a prop because a table that renders any collection is the one - * thing that cannot know what the collection is called, and a shared component - * carrying one collection's nouns would be shared in name only. The same - * argument is what grew the object from the five entries it opened with to every - * word below: a component holding one reader's language is shared in name only - * too. Nothing in this file is a literal a reader sees. - * - * Several entries are functions because they weave a value into a sentence. - * None of them takes a word: a caller handing over an already-composed phrase - * has made the grammatical decision one layer too early, which is exactly the - * defect the two sort entries below were rewritten to remove. The object is - * expected to hold one identity per language, so those closures stay stable - * across renders. - */ +/** Entries that weave a value take the value, never an assembled word. */ export interface DataTableLabels { /** Shown in place of the whole view until the rows have arrived once. */ readonly loading: string; @@ -63,16 +47,12 @@ export interface DataTableLabels { export interface DataTableProps { readonly rows: readonly T[]; readonly columns: readonly Column[]; - /** - * Must be injective. It keys the rows for reconciliation and it breaks ties - * between equal values in the sort, so two rows sharing a value here lose - * their identity and their ordering in the same stroke. - */ + /** Must be injective: it keys the rows and breaks ties in the sort. */ readonly getRowId: (row: T) => string; // The id is read off the column array above and only checked here. Without - // the wrapper the compiler would collect a candidate from this prop too and - // union it in, so a misspelt id would widen the union in silence instead of - // failing at the line that holds it. + // the NoInfer wrapper the compiler would collect a candidate from this prop + // too and union it in, so a misspelt id would widen the union in silence + // instead of failing at the line that holds it. readonly state: TableState>; readonly onSortChange: (columnId: NoInfer) => void; readonly onPageChange: (page: number) => void; @@ -80,12 +60,10 @@ export interface DataTableProps { readonly loading: boolean; // False until the underlying collection has arrived at least once. readonly datasetReady: boolean; - // The text of the failure rather than the failure itself. A component tier - // that renders a message cannot narrow an error object, which also means a - // preserved cause has no path to the screen from here. + // The text of the failure rather than the failure itself, so no component + // tier sees a cause. readonly errorMessage: string | null; - // Optional so the table stays usable on its own, without a container to - // re-run the request behind it. + // Optional so the table stays usable without a container behind it. readonly onRetry?: (() => void) | undefined; readonly labels: DataTableLabels; } @@ -96,11 +74,6 @@ export interface DataTableProps { * a11y: the failure arrives after the initial render, so without a live region * a screen reader user is never told the load failed or that a way back is on * offer. alert rather than status because the table it replaces is gone. - * - * A failed dataset load makes every search fail, so the way back belongs in the - * region that already reports it rather than in a second error surface. A - * native button carries the role, the focus, and the keyboard activation on its - * own. */ function ErrorRegion({ message, @@ -123,18 +96,7 @@ function ErrorRegion({ ); } -/** - * What the sort live region says. - * - * A cleared sort and a sort that has never been applied render the same state, - * so the first render has to stay silent while the press that clears a sort - * does not. That is what hasSorted separates. - * - * It separates the sorted case too, not just the cleared one: a sort can arrive - * without anybody pressing anything, because a link can carry one. That is - * still a first render, and a region reporting what the table is rather than - * what just changed announces something that did not happen. - */ +/** A link can carry a sort nobody pressed, so hasSorted gates the first. */ function sortAnnouncement( labels: DataTableLabels, sortDirection: "asc" | "desc" | null, @@ -143,23 +105,14 @@ function sortAnnouncement( ): string { if (!hasSorted) return ""; if (sortDirection && activeLabel) { - // The direction travels as the value it is rather than as a word chosen - // here. The word it turns into is a fact about a language, and the - // suffixed token this replaced was a word in exactly one of them. + // The direction travels as the value it is, not as a word chosen here: + // which word it becomes is a fact about a language. return labels.sortedAnnouncement(activeLabel, sortDirection); } return labels.sortClearedAnnouncement; } -/** - * What the results live region says. - * - * Silent unless there is a settled result to report, because announcing a count - * mid-request would name rows that are about to be replaced. The empty result - * gets a sentence of its own: emptying a region is not an announcement, so a - * search matching nothing would otherwise be indistinguishable from a request - * that never came back. - */ +/** Silent unless settled: a count mid-request names rows about to go. */ function resultsAnnouncement( labels: DataTableLabels, settled: boolean, @@ -171,10 +124,7 @@ function resultsAnnouncement( return labels.results(shown, total); } -/** - * The sort, described for the caption, in the words the caption weaves into a - * sentence rather than the words the live region announces. - */ +/** The sort described for the caption, in the caption's words, not the region's. */ function sortSummary( labels: DataTableLabels, sortDirection: "asc" | "desc" | null, @@ -185,17 +135,9 @@ function sortSummary( } /** - * Renders a collection as a sortable, paginated table. - * - * It holds nothing. The sort column and direction, the page position, the page - * size and the committed query all arrive in one object and leave as three - * calls describing what the user did, so the owner of that object decides what - * the next one is. Both derivations below are memos over modules that know - * nothing about React. - * - * Ordering and every rendered cell are delegated to the column descriptors, and - * every string naming what the rows are comes from the labels object, so - * nothing in this file knows which collection it is showing. + * Renders a collection as a sortable, paginated table, holding nothing itself. + * Cells come from the descriptors and words from the labels, so nothing here + * knows which collection it is showing. */ export function DataTable({ rows, @@ -225,29 +167,21 @@ export function DataTable({ state.pageSize, ); - // The announcements name what is on screen, and what is on screen is the - // column label. state.sortColumnId is the descriptor's id, which is not the name of - // anything the reader can see. - // Empty rather than absent when no column matches, so the two composers - // below hand a string to the catalog. The empty string is falsy exactly - // where the missing label was, which is what the announcement's guard reads. + // The announcements name the column label, not the descriptor's id. Empty + // rather than absent, so the composers below always hand over a string. const activeLabel = columns.find((column) => column.id === state.sortColumnId)?.label ?? ""; - // The four views the table can show, chosen once here rather than through a - // stack of conditional expressions inside the markup. The order is the - // precedence: a failure outranks a pending load, and both outrank a result. + // The four views the table can show. The order is the precedence: a failure + // outranks a pending load, and both outrank a result. let body: ReactNode; if (errorMessage !== null) { body = ( ); } else if (!datasetReady) { - // The whole view is replaced until the collection has arrived once, the - // first paint before the request even starts included: the empty result - // copy would otherwise claim a search had been made and matched nothing. - // Once the collection has arrived, a refetch keeps the table mounted so it - // does not unmount and flash on every keystroke. + // Gated on datasetReady rather than loading, or the empty-result copy + // would claim a search matched nothing before one was made. body =
{labels.loading}
; } else if (paginatedData.length === 0) { body =
{labels.empty}
; diff --git a/src/components/DataTable/Pagination.tsx b/src/components/DataTable/Pagination.tsx index 0800a63..ad59028 100644 --- a/src/components/DataTable/Pagination.tsx +++ b/src/components/DataTable/Pagination.tsx @@ -9,15 +9,7 @@ import { import { PAGE_SIZE_OPTIONS } from "./tableState"; import styles from "./Pagination.module.scss"; -/** - * Every string the page controls render. - * - * A slice of the table's own labels object rather than a second prop the caller - * assembles, so one object reaches the table and the table hands this part of it - * on. Each of the four action entries is used twice, once as the tooltip and - * once as the accessible name, which is what stops a translation moving one and - * leaving the other behind. - */ +/** Each action entry is read twice, as tooltip and as accessible name. */ export interface PaginationLabels { /** Names the control choosing how many rows a page holds. */ readonly pageSize: string; @@ -44,19 +36,7 @@ interface PaginationProps { readonly labels: PaginationLabels; } -/** - * The page-size control and the page navigation. - * - * It knows the page position and nothing else about the table's view state, so - * a change to how sorting is held cannot reach it. The select's value is parsed - * to a number here, at the only place that sees the event, so the callback - * never receives a string. - * - * Every word it shows arrives in the labels object, the table's own slice of - * which is handed down whole rather than spread or reshaped. Each of the four - * actions reads one entry twice, once as the tooltip and once as the accessible - * name, which is the only arrangement in which the two cannot drift apart. - */ +/** The page-size control and the page navigation, and no other view state. */ export function Pagination({ page, totalPages, @@ -65,15 +45,10 @@ export function Pagination({ onPageSizeChange, labels, }: PaginationProps) { - // Derived rather than a fixed string, so two tables on one page do not label - // each other's select. + // Derived, so two tables on one page do not label each other's select. const pageSizeId = useId(); - /** - * Turns the select's string value into the number the arithmetic divides by. - * The option list is a closed set, so nothing out of range or non-numeric can - * reach the arithmetic through this control. - */ + /** The option list is closed, so the parse cannot yield an unusable size. */ const handlePageSizeChange = (e: React.ChangeEvent) => { onPageSizeChange(Number.parseInt(e.target.value, 10)); }; diff --git a/src/components/DataTable/TableBody.tsx b/src/components/DataTable/TableBody.tsx index 29c587b..09a792f 100644 --- a/src/components/DataTable/TableBody.tsx +++ b/src/components/DataTable/TableBody.tsx @@ -7,15 +7,7 @@ interface TableBodyProps { readonly getRowId: (row: T) => string; } -/** - * The data rows: one row per element, one cell per column, each cell produced - * by that column's own renderer. - * - * The renderer's return value is inserted as a child, which the framework - * escapes, and the default renderer stringifies the value. That is the whole - * defence for a cell slot an author controls, and it is enough only for as long - * as no raw-markup insertion appears anywhere near it. - */ +/** The data rows, each cell produced by its own column's renderer. */ export function TableBody({ rows, columns, diff --git a/src/components/DataTable/TableHead.tsx b/src/components/DataTable/TableHead.tsx index 559f769..6e1b054 100644 --- a/src/components/DataTable/TableHead.tsx +++ b/src/components/DataTable/TableHead.tsx @@ -11,13 +11,7 @@ interface TableHeadProps { readonly onSortChange: (columnId: Id) => void; } -/** - * The header cell's sort state, in the three values the attribute accepts. - * - * Every column carries one: a sortable column that is not the active one - * reports "none" rather than omitting the attribute, so assistive technology - * describes it as sortable-but-unsorted instead of not sortable at all. - */ +/** An inactive column reports "none" rather than omitting the attribute. */ function ariaSort( direction: "asc" | "desc" | null, ): "ascending" | "descending" | "none" { @@ -26,15 +20,7 @@ function ariaSort( return "none"; } -/** - * The header row: one cell per column, each carrying the control that cycles - * the sort and the state the cycle is currently in. - * - * Everything rendered here comes from the descriptor, so this component names - * no field of the row type it orders. The width the descriptor may carry is - * applied as a style object rather than interpolated into an attribute, which - * is what keeps an author-supplied string off the attribute itself. - */ +/** The header row. The width is a style object, not an interpolation. */ export function TableHead({ columns, sortColumnId, diff --git a/src/components/DataTable/column.ts b/src/components/DataTable/column.ts index 1b2538f..abc8055 100644 --- a/src/components/DataTable/column.ts +++ b/src/components/DataTable/column.ts @@ -2,55 +2,22 @@ import type { ReactNode } from "react"; import { compareValues } from "../compareRows"; -/** - * A column, with its value type already fused in and then erased. - * - * The value type is deliberately absent from this interface. Keeping it as a - * second type parameter is the shape every table library reaches for, and it - * does not compile here: under strictFunctionTypes with property syntax, a - * column over a string value is not assignable to a column over an unknown one, - * so a heterogeneous array collapses the moment it is annotated or crosses a - * prop boundary. The method-syntax workaround compiles only because methods are - * bivariant, which is unsound, and it hands the cell renderer a value it cannot - * narrow. Fusing the value type into the two functions at construction and - * dropping it from the element type costs the table the ability to inspect a - * value, which the table never needed, and buys an array of columns over - * different value types that survives being passed around. - */ +/** Its value type is fused into the two functions below, then erased. */ export interface Column { - /** - * Unique within the array the column is rendered in. A repeat collides on - * the cell key React reconciles a row by and on the lookup that resolves the - * sort column. The builder below covers the array one builder produces, which - * is every array in this tree; concatenating two builders' output is not - * checked anywhere. - */ + /** Unique within its array: a repeat collides on React's own cell key. */ readonly id: Id; readonly label: string; - /** - * Declared now and set by nothing. Row virtualization needs a width it can - * measure without reading the DOM, and amending this interface twice is the - * cost that buys. - */ + /** Declared for a future row virtualizer and set by nothing today. */ readonly width?: string | undefined; - /** - * The column carries a number. The table decides what that looks like. - */ + /** The column carries a number. The table decides what that looks like. */ readonly numeric?: boolean | undefined; readonly renderCell: (row: T) => ReactNode; readonly compare: (a: T, b: T, direction: "asc" | "desc") => number; } /** - * What a caller supplies. Everything here is value-level: the accessor has - * already been applied by the time a renderer or a comparator declared here is - * called, which is what lets the value type be inferred once, at the call site, - * with no annotation. - * - * The comparator takes the direction rather than being flipped by its caller. - * Blanks sort last in both directions, which is a rule a direction-free - * comparator cannot express: negating it puts every blank first on descending, - * and on real data that is a first page of empty cells. + * What a caller supplies. The comparator takes the direction rather than being + * flipped by its caller, because blanks sort last in both. * * ponytail: a caller-supplied comparator keeps the three parameters it has * always had and never receives the collator, so a caller that wants to collate @@ -68,43 +35,14 @@ export interface ColumnOptions { } /** - * Builds columns for one row type. - * - * Curried because TypeScript infers all of a call's type arguments or none of - * them. The row type is the one thing a caller knows and the compiler cannot - * guess, so it is supplied here; the column id and the value type are then - * inferred per call, which is the whole point. - * - * The id is constrained to string and every call site passes a string literal, - * so it infers as that literal, not as string. An array of these carries the - * literal union of its ids with no assertion written anywhere, and renaming a - * column is a compile error at every use site rather than a silent widening. - * - * The const modifier on the two id parameters below is redundant, since a - * scalar string parameter already infers a literal from a literal argument. - * It changes inference only for a parameter that takes an array or object, - * which is not this signature. - * - * The collator is supplied here rather than reaching the comparison some other - * way, and it is what makes the column array the carrier of the reader's - * locale. The array is rebuilt per locale anyway, for its labels and for the - * cells that format a number, so fusing the collator in at construction costs - * one parameter and leaves the sort module, its hook and the table's own prop - * surface entirely untouched. A collator is a platform value, so taking one - * here leaves this layer's dependency set exactly as it was. - * - * One builder builds one table's columns, and throws on an id it has already - * issued. A second, unrelated table takes a second builder. + * Builds columns for one row type. Curried because TypeScript infers all of a + * call's type arguments or none, so an array carries its literal id union with + * no assertion. One builder per table: it throws on a repeated id. */ export function columns(collator: Intl.Collator) { const issued = new Set(); - /** - * The one place where the accessor is fused into the renderer and the - * comparator, so the two public methods below differ only in how they read a - * value. Supplying neither leaves the value stringified for display, blank - * if it is nullish, and ordered by the shared comparator. - */ + /** The one place the accessor is fused into the renderer and comparator. */ function build( id: Id, read: (row: T) => V, @@ -125,9 +63,8 @@ export function columns(collator: Intl.Collator) { renderCell: renderCell ? (row) => renderCell(read(row), row) : (row) => { - // Nullish paints an empty cell rather than the word "null". NaN - // is not covered: the comparator calls it blank, but a cell reading - // "NaN" is worth seeing. + // Nullish paints an empty cell rather than the word "null". NaN is + // not covered: the comparator calls it blank, but it is worth seeing. const value = read(row); return value == null ? "" : String(value); }, @@ -139,11 +76,7 @@ export function columns(collator: Intl.Collator) { } return { - /** - * A column whose id is one of the row type's own string keys, and whose - * value is that field. Constrained to the string keys because a number or - * symbol key cannot be a column id. - */ + /** A column over one of the row type's own string keys. */ key>( id: Id, options: ColumnOptions, @@ -151,10 +84,7 @@ export function columns(collator: Intl.Collator) { return build(id, (row) => row[id], options); }, - /** - * A column whose value is computed rather than read, so its id is free of - * the row type and its value type is inferred from the read function. - */ + /** A computed column: its id is free of the row type, its value inferred. */ accessor( id: Id, read: (row: T) => V, diff --git a/src/components/DataTable/sortRows.ts b/src/components/DataTable/sortRows.ts index 5418ade..76eb8a5 100644 --- a/src/components/DataTable/sortRows.ts +++ b/src/components/DataTable/sortRows.ts @@ -1,21 +1,6 @@ import type { Column } from "./column"; -/** - * Orders rows by one column, or hands them back untouched when nothing is - * sorted. - * - * The identity tiebreak lives here rather than inside a column's comparator - * because the function that produces a row's identity is a table-level prop: - * a column is defined without knowing what identifies a row, and this is the - * first place both are in scope at once. It runs after the column comparator - * has had its say, and it is never flipped by the direction, which keeps it one - * rule a reader can state in a line rather than a rule with an exception. It is - * also what makes sorting the same set twice produce the same order, rather - * than whichever order the rows happened to arrive in. - * - * What that tiebreak decides, and what the caller owes it, is on - * compareIdentities below. - */ +/** Row identity is a table-level prop, so the tiebreak lives here. */ export function sortRows( rows: readonly T[], column: Column | undefined, @@ -24,8 +9,8 @@ export function sortRows( ): readonly T[] { if (!column || !direction) return rows; - // The resolved collection is module-cached and shared by every reader, so - // it is treated as immutable and the sort runs over a copy. + // The resolved collection is module-cached and shared, so it is treated as + // immutable and the sort runs over a copy. return [...rows].sort((a, b) => { const comparison = column.compare(a, b, direction); if (comparison !== 0) return comparison; @@ -34,27 +19,7 @@ export function sortRows( }); } -/** - * Orders two row identities, ascending and never flipped by the direction. - * - * Identities order as text, so an identity of "100" comes before "99". That is - * a real ordering a reader can see, not a detail below the surface: it decides - * every pair whose column values compare equal, and a column with many equal - * values leaves most of the table to this rule. Supplying identities that sort - * as text the way their subjects sort is therefore the caller's job, and the - * caller is the only one who knows what a row's identity means. - * - * Deliberately not collated, and that is the one thing to keep true here. The - * column comparator above orders text by the reader's resolved locale, which is - * what a reader expects of the values they can see. An identity is not a value - * they can see: it decides every pair the column left tied, so collating it - * would let the same data come out in two orders for two readers with every - * visible value equal. Ordering identities as plain text is what keeps that one - * rule the same for everyone. - * - * Exported so a caller reproducing the table's order compares identities the - * way the table does, rather than restating the rule and drifting from it. - */ +/** Never flipped and never collated: an identity is not a visible value. */ export function compareIdentities(aId: string, bId: string): number { if (aId === bId) return 0; return aId < bId ? -1 : 1; diff --git a/src/components/DataTable/tableState.ts b/src/components/DataTable/tableState.ts index 51656be..ce67c6a 100644 --- a/src/components/DataTable/tableState.ts +++ b/src/components/DataTable/tableState.ts @@ -1,21 +1,11 @@ -/** - * Everything the table remembers between renders, in one object. - * - * It is one object rather than five values because it is also what gets read - * whole: a serialiser writes all of it at once and a restored address writes - * all of it back at once, and five separate writes are five chances to tear. - */ +/** Everything the table remembers. Five separate writes would be five tears. */ export interface TableState { readonly sortColumnId: Id | null; readonly sortDirection: "asc" | "desc" | null; readonly page: number; readonly pageSize: number; readonly query: string; - /** - * A cleared sort and a sort that has never been applied render the same - * state, so the announcement needs to know which of the two it is looking at: - * the first render has to stay silent, the third press on a column does not. - */ + /** A cleared sort and a sort never applied otherwise look the same. */ readonly hasSorted: boolean; } @@ -25,16 +15,7 @@ export type TableAction = | { readonly type: "pageSize"; readonly pageSize: number } | { readonly type: "query"; readonly query: string }; -/** - * Where a table starts, with three readers: the state below it, whatever - * serialises the state (a value equal to one of these is the value to leave - * out), and whatever parses it back (a parameter that fails validation falls - * back to the value here). One owner is what keeps those three agreeing. - * - * Typed over no column id at all, which makes it assignable to a table state - * over any id union, since the only place an id appears is a field that may - * also be null. - */ +/** Where a table starts. Typed over no id, so it fits any id union. */ export const DEFAULT_TABLE_STATE: TableState = { sortColumnId: null, sortDirection: null, @@ -44,26 +25,10 @@ export const DEFAULT_TABLE_STATE: TableState = { hasSorted: false, }; -/** - * The page sizes the table offers, in the order it offers them. - * - * One owner because this list is two things at once: the surface a reader picks - * from, and the rule that decides whether a size arriving from outside the - * application is one the table can represent. Two independent copies of a list - * with no mechanism keeping them in step is a failure mode this project has - * already filed once, and it costs one line here to avoid. - * - * Not narrowed with a const assertion: the membership test reads an arbitrary - * number, so a union of the four literals would be a type the caller cannot - * hand a value to. - */ +/** The page sizes offered, and the rule validating a size from outside. */ export const PAGE_SIZE_OPTIONS: readonly number[] = [10, 25, 50, 100]; -/** - * The fields an action changes, before the page reset is applied over them. - * Split out so the reset below can be written once for all three actions that - * trigger it rather than repeated inside each of their branches. - */ +/** The fields an action changes, before the shared page reset lands on top. */ function changedBy( state: TableState, action: Exclude, { type: "page" }>, @@ -75,10 +40,8 @@ function changedBy( return { query: action.query }; case "sort": { const { columnId } = action; - // Sorting cycles rather than toggling: a new column starts ascending, the - // active one goes ascending to descending to unsorted, and a cleared - // column starts over. Every branch reports that a sort happened, - // including the one that clears it. + // Sorting cycles rather than toggling: ascending, descending, unsorted, + // then over. Every branch reports a sort, the one that clears included. if (state.sortColumnId !== columnId) { return { sortColumnId: columnId, @@ -97,42 +60,27 @@ function changedBy( } } -/** - * The only thing that moves the table from one state to the next. - * - * It is pure and knows nothing about React, so the rules below can be read and - * tested without rendering anything. - */ +/** The only thing that moves the table from one state to the next. */ export function applyTableAction( state: TableState, action: TableAction, ): TableState { if (action.type === "page") { - // Taken exactly as given, never corrected against the pages that happen to - // exist. Correcting here would store a position the user did not choose, so - // a result set that widens again could not restore them, and a position - // arriving before its rows do would be corrected against no rows at all. - // The correction belongs where the rows are counted, and it belongs to the - // read rather than to the write. + // Taken exactly as given. Correcting here would store a position the reader + // did not choose, and a position arriving before its rows would be + // corrected against no rows at all. The clamp belongs to the read. return { ...state, page: action.page }; } - // A term that settles back where it started leaves the same rows in the same - // order, so the position chosen against them still means what it meant. This - // is the one action a control can emit at its current value: a sort press and - // a size selection are always a real change, while the debounce commits any - // sequence of keystrokes that pauses, including one that undoes itself. The - // same state object rather than an equal one, so the render and the address - // write behind it never run for a change that did not happen. + // The one action a control can emit at its current value, because the + // debounce commits any pause, including a sequence that undoes itself. The + // same object rather than an equal one, so no render and no address write. if (action.type === "query" && action.query === state.query) { return state; } - // Sorting, resizing the page, and searching each replace the set of rows the - // position was chosen against, so all three return to the first page. Fused - // into one return so that reset has exactly one site in the codebase: whoever - // adds a fifth action has to decide about it rather than forget it. The guard - // above is a guard against the action, not a condition on the reset: once an - // action reaches this line it resets, whatever it carries. + // Sorting, resizing and searching each replace the rows the position was + // chosen against, so all three return to the first page. One site, so whoever + // adds a fifth action has to decide about the reset rather than forget it. return { ...state, ...changedBy(state, action), page: 1 }; } diff --git a/src/components/DataTable/tableStateUrl.ts b/src/components/DataTable/tableStateUrl.ts index 9fcfe51..0cbbc3f 100644 --- a/src/components/DataTable/tableStateUrl.ts +++ b/src/components/DataTable/tableStateUrl.ts @@ -4,34 +4,10 @@ import { type TableState, } from "./tableState"; -/** - * What a descending sort is written with, ahead of the column id. - * - * One key rather than a column and a direction, and not to shorten the link: - * the two fields are coupled in the state, where the direction is null exactly - * when the column is, so a single token makes the invalid pair unrepresentable - * rather than merely rejected and the parser needs no cross-field rule at all. - * - * A hyphen rather than a colon-separated form, which has the same validation - * property but is percent-encoded in some of the paths a link travels, so the - * pasted address reads worse for no gain. - */ +/** One key rather than two, so the invalid pair is unrepresentable. */ const SORT_DESCENDING_PREFIX = "-"; -/** - * One parameter this schema owns: the key it answers to, how a raw value is - * read back into view state, and how the state's value for it is written out. - * - * The reading half returns a partial rather than a single value, so a key that - * carries two coupled state fields needs no second shape and no cross-field - * rule after the fact. The writing half returns null for a value equal to its - * default, so the rule that keeps defaults out of the address is applied once - * over the table below rather than restated inside every entry. - * - * Both halves are generic over the column ids per call rather than over the - * interface, which is what lets the schema be one module-scope array shared by - * every table instead of one array per row type. - */ +/** Parsing returns a partial; serializing returns null for a default. */ interface UrlParamEntry { readonly key: string; readonly parse: ( @@ -43,30 +19,14 @@ interface UrlParamEntry { ) => string | null; } -/** - * The parameters this application owns, in the order it writes them. - * - * The array's order is the canonical order, so there is no second list of key - * names to keep in step with it, and adding a parameter is one entry rather - * than an edit to a parser, a serializer, and an order. - */ +/** The parameters this application owns, in the canonical write order. */ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ { key: "q", - // Taken exactly as it arrives, with nothing to validate. A term is a free - // string by nature: it reaches a controlled input's value and a substring - // match over the collection, never a lookup and never markup, so there is - // no shape it could fail to have. Every rule a term does have, the spelling - // of a space and the encoding of the punctuation the query string uses for - // itself, belongs to the query serializer below rather than here. + // A term reaches a controlled value and a substring match, never a lookup. parse: (raw) => ({ query: raw }), - // Trimmed on the way out, because the search itself trims: a term carrying - // edge whitespace selects the same rows as one without, so writing it - // verbatim would give one view two addresses that never converge, and a - // term that is nothing but whitespace would write a key for a view - // identical to the default. Trimmed here rather than in the state, so what - // the reader is typing stays painted in the box exactly as they typed it - // while the address stays canonical. + // Trimmed on the way out because the search trims, so one view cannot have + // two addresses. Not trimmed in the state, so the box shows what was typed. serialize: (state) => { const term = state.query.trim(); return term === DEFAULT_TABLE_STATE.query ? null : term; @@ -74,19 +34,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "sort", - // The token is checked by locating it among the ids the caller supplied, - // comparing values rather than indexing anything. Located with find rather - // than tested and then asserted, so the result arrives already typed as one - // of those ids and nothing here has to claim a type for a string that came - // out of the address. - // + // Located with find, so the result arrives already typed as a caller's id. // The whole token is tried before the prefix is stripped, because an id may - // itself begin with the prefix and nothing constrains it not to. Stripping - // first makes such an id unreachable ascending: it writes a token that - // reads back as a different id descending, so the sort is silently dropped - // rather than restored. Trying the exact match first leaves exactly one - // ambiguity, between an id and the id the prefix would produce from it, and - // resolves it toward the one that exists as written. + // begin with it: stripping first leaves such an id unreachable ascending. parse: (raw, validColumnIds) => { const ascending = validColumnIds.find((candidate) => candidate === raw); if (ascending !== undefined) { @@ -111,19 +61,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "page", - // Coerced whole rather than with the radix parser, which truncates in two - // opposite directions: it reads exponent notation as a single digit and a - // number carrying trailing text as its numeric prefix. The page-size select - // can use it safely because its input is a fixed option list; the address - // is not a fixed list. - // - // Any positive integer is taken, with no upper bound. The clamp that turns - // a position into rows is what bounds it, and an out-of-range value has to - // survive in the address rather than be corrected back into it. That also - // means hexadecimal notation is accepted as the number it denotes, which is - // a choice rather than an oversight: it is a positive integer, the clamp - // bounds it, and a format rule here would be a rule with no failure it - // prevents. + // Coerced whole rather than with the radix parser, which reads exponent + // notation as a single digit. Any positive integer, with no upper bound: + // the read-side clamp is what bounds it. parse: (raw) => { const page = Number(raw); return Number.isInteger(page) && page > 0 ? { page } : undefined; @@ -133,12 +73,8 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ }, { key: "size", - // Accepted only when it is a size the table offers, because the table's own - // select cannot represent one that is not among its options: accepting an - // arbitrary size would render a control whose value is not in its own list. - // Membership in that list already implies a whole number, so there is no - // second predicate to write. Coerced whole for the same reason the position - // above is. + // Only a size the table offers, because the select cannot represent one + // that is not among its options. Membership already implies a whole number. parse: (raw) => { const pageSize = Number(raw); return PAGE_SIZE_OPTIONS.includes(pageSize) ? { pageSize } : undefined; @@ -151,17 +87,9 @@ const PARAM_SCHEMA: readonly UrlParamEntry[] = [ ]; /** - * Reads whatever of the view state a query string carries. - * - * Total by construction: a value that fails validation is left out rather than - * replaced, and the caller spreads the result over the default state, so an - * omitted field is the default and no parameter needs a fallback of its own. - * - * The valid column ids arrive as an argument rather than being imported, which - * is what keeps this module ignorant of what its rows are, exactly as the table - * receives its columns. Nothing read out of the query is ever used as an object - * key and nothing is deep merged, so a parameter named after a prototype member - * is structurally harmless rather than a case in a validator. + * Reads whatever of the view state a query string carries. Total by + * construction: a value failing validation is left out, so no parameter needs a + * fallback arm. The column ids arrive as an argument. */ export function parseTableState( search: string, @@ -171,9 +99,8 @@ export function parseTableState( const restored: Partial> = {}; for (const entry of PARAM_SCHEMA) { - // The first occurrence of a repeated key, which is the whole rule for one: - // the extras are dropped by the write that follows rather than by a rule of - // their own. + // The first occurrence of a repeated key; the extras are dropped by the + // write that follows rather than by a rule of their own. const raw = params.get(entry.key); if (raw === null) continue; @@ -187,15 +114,9 @@ export function parseTableState( } /** - * Writes the view state back out, preserving every parameter it does not own. - * - * Owned keys are written first in the schema's order and anything else follows - * in the order it arrived, so two equivalent views produce the same string - * while a tracking tag someone else put in the link survives the write. - * - * The result is shaped the way the address bar's own query is shaped: empty - * when there is nothing to say, and otherwise a question mark followed by the - * parameters. That is what lets the caller's write guard be a bare comparison. + * Writes the view state back out, preserving every parameter it does not own + * and shaped like the address bar's own query, so the write guard is a bare + * comparison. */ export function serializeTableState( state: TableState, @@ -212,8 +133,8 @@ export function serializeTableState( } for (const [key, value] of incoming) { - // Ownership is decided by comparing against the schema's own key strings, - // never by looking the incoming key up in an object. + // Ownership is decided by comparing key strings, never by looking the + // incoming key up in an object. if (!PARAM_SCHEMA.some((entry) => entry.key === key)) { next.append(key, value); } @@ -223,15 +144,7 @@ export function serializeTableState( return query === "" ? "" : `?${query}`; } -/** - * Reads only the search term a query string carries. - * - * The container behind the table needs the term on its very first render and - * owns none of the columns, so it reads through this rather than passing an - * empty column-id list to say it does not care about the sort. One schema and - * two entry points: the rules still live in exactly one place, and a reader of - * the call site can see what is being asked for. - */ +/** Reads only the term, for the container that owns none of the columns. */ export function parseSearchTerm(search: string): string { return parseTableState(search, []).query ?? DEFAULT_TABLE_STATE.query; } diff --git a/src/components/SearchInput.tsx b/src/components/SearchInput.tsx index c0a7916..84a58ef 100644 --- a/src/components/SearchInput.tsx +++ b/src/components/SearchInput.tsx @@ -2,12 +2,7 @@ import { FiSearch } from "react-icons/fi"; import styles from "./SearchInput.module.scss"; -/** - * The two strings this control shows. - * - * One object rather than two loose props, so the pair moves together when the - * language does and a caller cannot supply half of it. - */ +/** The two strings this control shows, as one object so they move together. */ export interface SearchInputLabels { /** The accessible name: what the control does, not what it searches. */ readonly name: string; @@ -22,12 +17,9 @@ interface SearchInputProps { } /** - * The search box above the table. - * - * The term is reported upward rather than held here, so the control stays a - * pure function of the value its owner already has. The event-to-term - * conversion happens here, at the only place that knows an input event exists, - * which keeps the callback signature free of the DOM. + * The search box above the table. The term is reported upward rather than held + * here, and the event-to-term conversion happens at the only place that knows + * an input event exists, which keeps the callback signature free of the DOM. * * a11y: the accessible name describes what the control does rather than what it * searches, which is why it is one word and not the name of a collection. That @@ -38,10 +30,7 @@ interface SearchInputProps { * the caller knows. */ export function SearchInput({ value, onChange, labels }: SearchInputProps) { - /** - * Reports every keystroke upward. Debouncing belongs to whoever owns the - * request, not to the control. - */ + /** Reports every keystroke upward; debouncing belongs to the request owner. */ const handleSearchChange = (e: React.ChangeEvent) => { onChange(e.target.value); }; diff --git a/src/components/compareRows.ts b/src/components/compareRows.ts index 2aaddb4..bf95d64 100644 --- a/src/components/compareRows.ts +++ b/src/components/compareRows.ts @@ -1,33 +1,16 @@ -/** - * A value carrying nothing to order by. A zero is deliberately not blank; the - * dataset records hundreds of cities with no population as 0, and those rows - * belong at the small end of a population sort rather than at the far end with - * the empty strings. - * - * NaN is blank. It is a number by typeof, so without this it reaches the - * subtraction and returns NaN, which is neither negative, positive, nor zero: - * the direction flip leaves it NaN, the row-id tiebreak never runs, and the - * order of a set holding one comes out different for different arrival orders. - */ +/** Nothing to order by. Zero is not blank (0 population sorts small); NaN is. */ function isBlank(value: unknown): boolean { return ( value === "" || value === null || value === undefined || Number.isNaN(value) ); } -/** - * Where a value's type sits in the ordering. Grouping by type before comparing - * within it is what keeps the comparison transitive once the rows stop being - * uniformly typed: numbers order among themselves, everything else collates as - * text, and the two groups never have to be compared by a rule that disagrees - * with the rule used inside them. - */ +/** Grouping by type keeps the comparison transitive over mixed rows. */ const TYPE_RANK = { number: 0, string: 1, other: 2 } as const; /** - * Places one value in the ordering above, dispatching on its runtime type - * because the rows reach here already widened and a declared type is not - * available to dispatch on. + * Places a value in the ordering above. The parse boundary types every City + * field, so only a row type other than City reaches the mixed arms. */ function rank(value: unknown): number { if (typeof value === "number") return TYPE_RANK.number; @@ -35,13 +18,7 @@ function rank(value: unknown): number { return TYPE_RANK.other; } -/** - * Orders two values that are both present, before any direction is applied. - * - * Type is the primary key, so which rule decides a pair is settled by the pair's - * types rather than by the values: without that, a numeric pair and a - * stringified pair can each be decided by a different rule and produce a cycle. - */ +/** Orders two present values. Type is the primary key, so no pair cycles. */ function compareRanked( aValue: unknown, bValue: unknown, @@ -52,11 +29,8 @@ function compareRanked( if (aRank !== bRank) return aRank - bRank; if (aRank === TYPE_RANK.number) { - // Compared rather than subtracted. Two infinities of the same sign subtract - // to NaN, which would skip the identity tiebreak the sort module applies - // after this returns and leave the order up to the sort, and a subtraction - // of two large magnitudes reports a difference where a direction is all - // that is wanted. + // Compared rather than subtracted: two infinities of the same sign subtract + // to NaN, which would skip the sort module's identity tiebreak. const aNumber = aValue as number; const bNumber = bValue as number; if (aNumber === bNumber) return 0; @@ -67,40 +41,9 @@ function compareRanked( } /** - * Orders two already-widened values. - * - * The three rules below compose in an order that is load-bearing, so read them - * as a sequence rather than as a set. - * - * The blank test runs first, ahead of the direction flip, which is what puts - * blanks last whichever way the column is sorted. Capital is empty on roughly - * two thirds of the rows, so the other rule leads the first ascending page with - * a screen of empty cells and reads as a broken table. - * - * The typed comparison runs second, and its result is the only thing the - * direction flip touches. Type is the primary key there, so which rule decides - * a pair is settled by the column rather than by the pair: without that, a - * numeric pair and a stringified pair can each be decided by a different rule - * and produce a cycle, which is an ordering the sort is free to resolve however - * it likes. The parse boundary guarantees each field's type today, so the - * dataset cannot reach the mixed arms; they are here for the point at which the - * table becomes generic over its row type and that guarantee stops covering the - * input. - * - * The row-identity tiebreak that used to run last is no longer here. It ran on - * a field of the row, and this function no longer sees a row: identity is a - * table-level prop, not a fact a column can know about itself. It now runs in - * the sort module, after this function returns and never flipped, so the rule - * itself is unchanged. - * - * The collator arrives as an argument, and it has no default. This module held - * one for its own lifetime while the ordering was whatever the machine running - * the code happened to prefer; text now collates by the reader's resolved - * locale, so the instance is a function of that locale and cannot be a constant - * here. A default would be the locale-less collator this parameter exists to - * remove, and it would hide a call site from the source guard that keeps every - * platform locale construction in one module. This layer still imports nothing - * from the locale layer: a collator handed in is a value, not a dependency. + * Orders two already-widened values. Blanks are tested ahead of the direction + * flip so they stay last either way, and the collator is a parameter because + * this layer may not import the locale layer. */ export function compareValues( aValue: unknown, @@ -112,14 +55,11 @@ export function compareValues( const bBlank = isBlank(bValue); if (aBlank !== bBlank) return aBlank ? 1 : -1; - // Both blank compares equal: the arm above has already answered every pair - // where only one of them is. + // Both blank compares equal; the arm above answered every one-sided pair. const comparison = aBlank ? 0 : compareRanked(aValue, bValue, collator); - // Returned ahead of the flip so an equal pair comes back as a positive zero - // in both directions. Negating zero gives negative zero, which every ordering - // rule downstream reads as a tie but which an equality check does not: the - // pair would compare equal ascending and not equal descending. + // Returned ahead of the flip so an equal pair is a positive zero either way. + // Negating zero gives -0, which an equality check downstream reads as unequal. if (comparison === 0) return 0; return direction === "desc" ? -comparison : comparison; } diff --git a/src/components/paginate.ts b/src/components/paginate.ts index cbe8e6f..0f9d41c 100644 --- a/src/components/paginate.ts +++ b/src/components/paginate.ts @@ -1,35 +1,20 @@ -/** - * One page read out of a collection: the rows on that page, how many pages the - * collection has, and which page was actually read. - */ +/** One page read out of a collection: rows, page count, page actually read. */ export interface PaginateResult { readonly paginatedData: readonly T[]; readonly totalPages: number; readonly effectivePage: number; } -/** - * Slices one page out of a collection. - * - * The page position is an argument rather than something this function owns, - * and the correction it applies below never leaves this function, which is what - * lets a position arrive from anywhere: a click, a restored URL, or a render - * that happens before the rows do. - */ +/** Slices one page. The position is an argument; the clamp stays here. */ export function paginate( rows: readonly T[], page: number, pageSize: number, ): PaginateResult { - // Floored at one so an empty result set still counts as a single page. - // Zero would be a page count nothing can be on, and callers outside the - // navigation's own visibility guard have no protection from it. + // Floored at one: zero would be a page count nothing can be on. const totalPages = Math.max(1, Math.ceil(rows.length / pageSize)); - // Clamped for reading only. The position held in state is deliberately - // left alone, so a result set that widens again restores the user where - // they were rather than stranding them on whatever the narrowed set - // allowed, and so a position arriving from outside survives a fetch that - // has not resolved yet. + // Clamped for reading only. The position in state is left alone, so a result + // set that widens again restores the reader where they were. const effectivePage = Math.min(Math.max(page, 1), totalPages); const startIndex = (effectivePage - 1) * pageSize; return { diff --git a/src/data/worldcities/cities.ts b/src/data/worldcities/cities.ts index 0a82b44..f3afa7d 100644 --- a/src/data/worldcities/cities.ts +++ b/src/data/worldcities/cities.ts @@ -1,18 +1,13 @@ -// The ?url suffix is load-bearing. A plain value import of the same file would -// compile the whole dataset back into the JavaScript bundle, with no visible -// error, and the app would then download it a second time as well. +// The ?url suffix is load-bearing: a plain value import would compile several +// megabytes of dataset into the JavaScript chunk with no visible error. import citiesUrl from "./cities.json?url"; /** * simplemaps.com "World Cities" basic database, v1.91.3, distributed under * CC BY 4.0. See license.txt. * - * The dataset ships as cities.json. Only the columns the City type needs are - * kept; lat, lng, iso2, and admin_name are dropped. Rows are ordered by - * descending population so the default view leads with the largest cities. - * - * Provenance is recorded in license.txt and in the README rather than here, so - * there is one account of how the committed bytes came to exist. + * Only the columns this type needs are kept, and rows are ordered by descending + * population. Provenance is recorded in license.txt and the README, not here. * * Upstream quirks preserved deliberately: * - 432 rows have no population and are recorded as 0. @@ -26,10 +21,8 @@ import citiesUrl from "./cities.json?url"; * translated column and a regenerated asset, which is a data pipeline rather * than an internationalization change. * - * That is stated here as well as in the README because this is the file a reader - * asking why a name is not translated is already looking at. The two copies are - * held together by a guard in src/toolchain.test.ts, so neither can be reworded - * on its own. + * Stated here as well as in the README, and held together by a guard in + * src/toolchain.test.ts so neither copy can be reworded on its own. */ export interface City { id: number; @@ -42,19 +35,10 @@ export interface City { } /** - * The ways loading the dataset can fail, as a closed set of codes. - * - * A code rather than a message, because the message below is English and the - * reader may not be. The application layer turns a code into the sentence a - * reader sees, and this module still imports nothing but its own asset: the - * codes are its own vocabulary in the way the city type is. - * - * Eight of the nine are thrown here. The ninth is the container's own fallback - * for a rejection that carries no error at all, which cannot originate here - * because nothing here rejects with a non-error. - * - * A tuple rather than a bare union, so the catalog test can walk the set rather - * than restate it. + * The ways loading can fail. Codes, because the messages below are English. + * Eight are thrown here; "unexpected" is App's fallback for a rejection that + * carries no Error. A tuple, so the catalog test walks the set rather than + * restating it. */ export const DATASET_ERROR_CODES = [ "notAnObject", @@ -71,18 +55,7 @@ export const DATASET_ERROR_CODES = [ /** Which failure a dataset error is. */ export type DatasetErrorCode = (typeof DATASET_ERROR_CODES)[number]; -/** - * A dataset failure, carrying the code that says which one it is. - * - * The message stays exactly what it was and stays English: it is the - * developer-facing text, the one a stack trace and a test assertion read. The - * preserved cause stays where it was attached. Neither reaches the screen. - * - * The detail is a single number and every failure carries one, including the - * six whose sentence ignores it. Uniform on purpose, so the lookup that turns a - * code into a sentence has one shape and no branch; the three that use it carry - * a row index or a response status. - */ +/** A dataset failure and its code. The message never reaches the screen. */ export class DatasetError extends Error { constructor( readonly code: DatasetErrorCode, @@ -95,11 +68,7 @@ export class DatasetError extends Error { } } -/** - * The column order the asset must declare. Asserting it is what converts the - * tuple shape's one real defect from silently mis-mapping 50,250 rows into a - * loud startup failure. - */ +/** The order the asset must declare, so a mis-mapping is a startup failure. */ const COLUMNS = [ "id", "name", @@ -111,41 +80,21 @@ const COLUMNS = [ ] as const; /** - * Separates the fields of the derived search key. A text input cannot produce - * this character, so a needle can never match across a field boundary. - * Concatenating with a space instead would diverge from the per-field matcher - * this replaces on thousands of rows. + * Joins the indexed fields. The address can carry one (`?q=%00`) and trim does + * not strip it, so getCities removes it from the needle rather than this being + * a character no input produces. */ -const SEARCH_KEY_SEPARATOR = "\u0000"; +export const SEARCH_KEY_SEPARATOR = "\u0000"; -/** - * The derived key is a search cache, not a fact about a city, so it stays off - * the exported type: a searchKey on City would surface in a column descriptor - * as a column-shaped field that is not a column. - */ +/** A search cache rather than a fact about a city, so it stays off City. */ interface IndexedCity extends City { searchKey: string; } -/** - * How long the whole download gets before a stall counts as a failure. Without - * it a stalled request never rejects and the reader waits on a spinner with - * nothing behind it. - * - * Deliberately generous, because this is a wall-clock deadline over a 3.3MB - * asset with no resume: a retry restarts the download from zero against the - * same budget, so a link too slow to finish inside it fails every attempt - * rather than merely being slow. A minute clears roughly 140kbps sustained, - * which is below any link that could have finished before this existed. - */ +/** Generous: a wall-clock deadline over a 3.3MB asset that cannot resume. */ const LOAD_TIMEOUT_MS = 60_000; -/** - * A download that did not finish, whichever way it stopped. A stall and a - * dropped connection are one failure to a reader, whose next move is the retry - * either way, so they share a code and a sentence rather than splitting the - * catalog. - */ +/** A download that did not finish. A stall and a drop are one failure. */ function transportError(cause: unknown): DatasetError { return new DatasetError( "transport", @@ -157,12 +106,7 @@ function transportError(cause: unknown): DatasetError { let cached: Promise | undefined; -/** - * The only place the untyped result of response.json() is narrowed. Each - * failure carries a code, and the sentence a reader is shown is chosen from - * that code one layer up; the message written here is the developer-facing - * text and no longer reaches the screen. - */ +/** The only place the untyped result of response.json() is narrowed. */ function parseCities(payload: unknown): IndexedCity[] { if (typeof payload !== "object" || payload === null) { throw new DatasetError( @@ -230,12 +174,8 @@ function parseCities(payload: unknown): IndexedCity[] { countryIso3, capital, population, - // Four of the five rendered columns. Capital is the one left out, and - // deliberately: its only values are the upstream classification codes - // "primary", "admin", "minor", and empty, which nobody types into a city - // search, and folding them in would bleed into every short needle. Over - // the committed rows "in" would match 31,388 of 50,250 instead of 19,051, - // through "admin" and "minor" rather than through anything a reader meant. + // Four of the five rendered columns. Capital is left out: its upstream + // codes would make "in" match 31,388 rows of 50,250 instead of 19,051. searchKey: [name, nameAscii, country, countryIso3] .join(SEARCH_KEY_SEPARATOR) .toLowerCase(), @@ -243,23 +183,15 @@ function parseCities(payload: unknown): IndexedCity[] { }); } -/** - * Downloads, validates, and indexes the dataset, caching the promise at module - * scope. A cache hit returns the same promise, which is what makes a double - * mount issue one request by construction rather than by cancellation. - */ +/** The module-scope cache is what makes a double mount issue one request. */ export function loadCities(): Promise { if (cached) return cached; const pending = fetch(citiesUrl, { signal: AbortSignal.timeout(LOAD_TIMEOUT_MS), }) - // The text a request carries when it never reaches the host is the - // browser's own, it differs between browsers, and none of it tells the - // reader what to do. It is replaced here and kept as the cause. This is - // attached to the request rather than to the chain below, so nothing - // thrown while reading the response can be reported as a transport - // failure. + // The browser's own text tells the reader nothing, so it is replaced and + // kept as the cause. Attached here, so a read failure is not reported as one. .catch((reason: unknown) => { throw transportError(reason); }) @@ -271,19 +203,11 @@ export function loadCities(): Promise { `The city data could not be downloaded (status ${response.status}).`, ); } - // A static host that serves the application's own page for a file it - // cannot find answers with a success status and a body of HTML, so the - // parser reports a syntax error naming a character rather than anything - // the reader can act on. The status check stays ahead of this, so a - // status failure is never reported as a parse failure. + // A static host serving its own page for a missing file answers with a + // success status and HTML, so the status check stays ahead of this. return response.json().catch((reason: unknown) => { // A body that is not JSON fails the parse and nothing else does, so - // the parse failure is the narrow case and everything else here is the - // download stopping partway: the timeout covers the body read, and a - // socket dropped after the headers arrived rejects here too. Both are - // failed downloads, and reporting either as an unreadable file would - // send the reader looking for a corrupt asset instead of at their - // connection. + // everything else reaching here is the download stopping partway. if (!(reason instanceof SyntaxError)) { throw transportError(reason); } @@ -297,13 +221,9 @@ export function loadCities(): Promise { }) .then(parseCities); - // Attached at store time and never deferred. Any delay leaves a window in - // which a retry re-awaits the already-rejected promise and fails instantly - // for a reason the user cannot see. The clear is unconditional because the - // only path that stores a new entry runs after this handler has already - // cleared the old one, so a rejection can never reach an entry other than - // its own. The rejection still reaches callers, who hold the promise - // returned below rather than this derived one. + // Attached at store time: any delay leaves a window in which a retry + // re-awaits the already-rejected promise. Unconditional is safe, because a + // new entry is only stored after this handler has cleared the old one. pending.catch(() => { cached = undefined; }); diff --git a/src/features/CityTable/CityTable.tsx b/src/features/CityTable/CityTable.tsx index e794aa3..9409c14 100644 --- a/src/features/CityTable/CityTable.tsx +++ b/src/features/CityTable/CityTable.tsx @@ -23,11 +23,7 @@ import { } from "./cityColumns"; import styles from "./CityTable.module.scss"; -/** - * How long typing has to pause before the term is committed. The window is the - * one the container applied before this component took the search box over, so - * the move retunes nothing. - */ +/** How long typing has to pause before the term is committed. */ const SEARCH_DEBOUNCE_MS = 150; interface CityTableProps { @@ -38,25 +34,17 @@ interface CityTableProps { readonly loading: boolean; // False until the underlying collection has arrived at least once. readonly datasetReady: boolean; - // The text of the failure rather than the failure itself, which is the shape - // the table below takes: nothing under this component narrows an error, so a - // preserved cause cannot reach a reader by accident. + // The text of the failure rather than the failure itself, so nothing below + // this component narrows an error and a cause cannot reach a reader. readonly errorMessage: string | null; - // Optional so the table stays usable on its own, without a container to - // re-run the request behind it. + // Optional so the table stays usable without a container behind it. readonly onRetry?: () => void; } /** - * The city table: the shared table wired to the city columns, the city copy, - * and the view state that drives them. - * - * Everything this application knows about cities that the table has to render - * is assembled here, which is what leaves the shared component free of it. - * The search box belongs to this component: it holds what is being typed and - * the term that typing settles on, and it reports the settled term upward so - * the container can issue the request behind it. Which fields a term matches is - * still decided at the data layer. + * The shared table wired to the city columns, the city copy and the view state + * that drives them. The search box belongs here: it holds what is being typed + * and the term typing settles on, and reports the settled term upward. */ export function CityTable({ data, @@ -67,45 +55,25 @@ export function CityTable({ onRetry, }: CityTableProps) { // The one place below the header that subscribes to the locale. Everything - // under src/components/ takes its strings as props and never learns that a - // locale exists, which is what keeps the shared table shared. + // under src/components/ takes its strings as props. const { catalog, tag } = useLocale(); - // The deliberate exception to the rule that label objects are built at module - // scope. The table holds this object across renders and two of its entries are - // closures, so its identity has to change when the locale does and must not - // change otherwise. That is exactly what a memo keyed on the catalog and the - // tag gives, and a module-scope constant cannot give it at all. + // The documented exception to module-scope label objects: the table holds + // this across renders and several entries are closures, so its identity has + // to move when the locale does and must not move otherwise. const labels = useMemo(() => buildTableLabels(catalog, tag), [catalog, tag]); - // The search box's own two strings, built from the same catalog on the same - // render so the whole tree changes language at once. Keyed on the catalog - // alone because neither entry weaves a number, so the tag decides nothing - // here and listing it would claim a dependency this does not have. + // Keyed on the catalog alone, because neither entry weaves a number and the + // tag decides nothing here. const searchLabels = useMemo(() => buildSearchLabels(catalog), [catalog]); - // The other documented exception to module-scope construction, and it keys on - // exactly the two values the labels above key on. That is a requirement - // rather than a symmetry: a column array whose identity moved on a render - // where the labels did not would re-sort the whole collection and re-slice - // the page for nothing, which over fifty thousand rows is the most expensive - // thing this component can do by accident. + // Keyed on exactly the two values the labels are, and that is a requirement + // rather than a symmetry: an array identity that moved on its own would + // re-sort fifty thousand rows for nothing. const columns = useMemo(() => buildCityColumns(catalog, tag), [catalog, tag]); // Initialized from whatever the address carries, so the first render is - // already the restored view: a link naming a page never paints the first one - // for a frame on the way there. Reading it here rather than in an effect is - // what buys that, and the initializer is pure, so the development-mode double - // invoke costs one extra parse. - // - // This component holds the view state rather than receiving it the way the - // shared table does, and the reason is measured rather than stylistic. About - // forty renders in the integration suite drive sorting and paging by clicking - // this component, and that suite is the accessibility regression record - // carried forward from an earlier phase. Hoisting the state would put a - // stateful wrapper under every one of them, at which point the suite asserts - // against the wrapper rather than against the application. Four test edits - // against roughly forty is the whole of the argument. + // already the restored view rather than the first page for a frame. const [tableState, setTableState] = useState>( () => ({ ...DEFAULT_TABLE_STATE, @@ -113,22 +81,13 @@ export function CityTable({ }), ); - // What is currently in the box, which is not yet what the table has been - // asked for. Seeded from the committed term, which the initializer above has - // already read out of the address, so a link carrying a term paints that term - // on the first render rather than filling the box in after mount. Declared - // next to the state it settles into so a reader meets the pair together. + // What is in the box, which is not yet what the table was asked for. Seeded + // from the committed term, so a link carrying one paints it on first render. const [searchInput, setSearchInput] = useState(tableState.query); // The only place in this application that writes the address, and it replaces - // rather than pushes, so one Back press leaves the site instead of stepping - // the reader back through positions they never asked to record. - // - // The comparison ahead of the write earns four things at once: a link that is - // already canonical is never rewritten, a parameter stating a default is - // stripped the moment it arrives, a hostile link is canonicalized on arrival, - // and a change driven by a back navigation cannot loop, because by then the - // address already says what the state says. + // rather than pushes. The comparison ahead of the write canonicalizes a + // hostile link on arrival and stops a back navigation looping. // // One address is one view, per resolved locale: the query string carries // the search term, the sort column and direction, the page and the page @@ -141,19 +100,13 @@ export function CityTable({ const next = serializeTableState(tableState, window.location.search); if (next === window.location.search) return; - // An empty query has to be written as the path. The empty string resolves - // to the current address and leaves the stale query exactly where it was, - // which is a write that reports success and changes nothing. The fragment - // rides along in both branches because a relative reference carrying a - // query but no fragment drops the fragment, and a shared link can carry one - // this application never put there. + // An empty query is written as the path: the empty string resolves to the + // current address and leaves the stale query where it was. The fragment + // rides along in both branches or a relative reference would drop it. // - // Guarded because no write to the address is worth the table. Browsers rate - // limit history mutation and throw rather than ignoring the call, and a held - // Enter key on the paging control reaches that ceiling in seconds over a - // collection with thousands of pages. A throw here is a throw in a - // commit-phase effect, so the boundary above would replace the whole view - // with the failure fallback over a link that failed to update. + // Guarded because no write to the address is worth the table. A browser + // that rate limits history mutation throws, and a throw in a commit-phase + // effect would replace the whole view with the failure fallback. try { window.history.replaceState( window.history.state, @@ -167,8 +120,7 @@ export function CityTable({ } }, [tableState]); - // The functional updater form is what keeps these dependency arrays empty, so - // the three callbacks keep one identity for the life of the table. + // The functional updater form is what keeps these dependency arrays empty. const handleSort = useCallback((columnId: CityColumnId) => { setTableState((state) => applyTableAction(state, { type: "sort", columnId }), @@ -185,28 +137,14 @@ export function CityTable({ ); }, []); - // This one carries a dependency where the three above carry none, so its - // identity is an argument rather than a guarantee: the only thing it depends - // on is the parent's callback, and that callback is itself memoized with an - // empty array, so in practice it is as stable as the three. Nothing below it - // inherits that argument, because the debounce reads its callback out of a - // ref rather than closing over it. - // - // Committing is the single point a pause in typing reaches. It moves the view - // state, which returns the reader to the first page because a new term is a - // different set of rows rather than a narrowing of the current one, and it - // reports the term upward so the request behind it is reissued. + // The single point a pause in typing reaches: it moves the view state, which + // returns the reader to the first page, and reports the term upward. Its one + // dependency does not reach the debounce, which reads its callback from a ref. const commitSearch = useCallback( (term: string) => { - // Canonicalized here, once, rather than at each of the three places that - // decide whether two terms are the same view. The search trims before it - // matches and the address trims before it writes, so a term differing - // only in edge whitespace selects the same rows at the same address; - // committing it verbatim is what makes the state disagree with both of - // them, and the disagreement costs the reader their position, strips the - // page from the address, and reissues a request for rows that did not - // change. The box goes on painting what was typed, because what is being - // typed is separate state from what typing settles on. + // Canonicalized once here, because the search trims before it matches + // and the address trims before it writes. The box goes on painting what + // was typed, which is separate state from what typing settles on. const settled = term.trim(); setTableState((state) => @@ -220,21 +158,13 @@ export function CityTable({ const { schedule: scheduleSearchCommit, cancel: cancelSearchCommit } = useDebouncedCallback(commitSearch, SEARCH_DEBOUNCE_MS); - // A history entry this application did not create can still carry a query it - // owns, so a traversal re-reads the whole view from the address and applies - // it in one write. Attached and detached symmetrically rather than assigned - // onto the window, so a second listener cannot silently replace this one. - // - // Declared after the scheduler rather than beside the write above, because it - // has to be able to cancel a commit the scheduler is still holding, and a - // dependency array is read during the render that declares it. + // A traversal re-reads the whole view from the address and applies it in one + // write. Declared after the scheduler, because it has to cancel a commit the + // scheduler is still holding. useEffect(() => { const handlePopState = () => { - // A traversal landing inside the debounce window would otherwise let the - // term the reader typed a moment ago land on top of the view they just - // navigated back to: the box would show the restored term while the rows, - // the position, and the address all carried the typed one. The keystrokes - // belong to the view the reader has left, so the commit goes with it. + // The keystrokes belong to the view the reader has left, so a commit + // still pending goes with it rather than landing on the restored view. cancelSearchCommit(); const restored: TableState = { @@ -244,20 +174,13 @@ export function CityTable({ setTableState((state) => ({ ...restored, - // A restored sort is still a first render, and announcing a sort to - // someone who has just opened a link announces something that did not - // just happen. Carrying the flag across means a traversal after a real - // sort still announces, while a cold one stays silent. It is the one - // field carried over, because it is the one field the address does not - // and will not hold. + // The one field carried over, because it is the one the address does + // not hold: a restored sort is still a first render and stays silent. hasSorted: state.hasSorted, })); - // The box and the request behind it both follow the term the traversal - // landed on. Reporting it upward rather than letting the container read - // the address for itself keeps the single writer single and the reader - // count at two, and it is why this handler depends on the parent's - // callback where the three above depend on nothing. + // Reported upward rather than read from the address by the container, + // which is what keeps the reader count at two. setSearchInput(restored.query); onSearchChange(restored.query); }; @@ -268,8 +191,7 @@ export function CityTable({ }; }, [onSearchChange, cancelSearchCommit]); - // The box repaints on every keystroke while the commit waits for the pause, - // so typing stays responsive and the table is asked once for what was typed. + // The box repaints on every keystroke while the commit waits for the pause. const handleSearchChange = useCallback( (term: string) => { setSearchInput(term); diff --git a/src/features/CityTable/cityColumns.ts b/src/features/CityTable/cityColumns.ts index 014fa2f..af23c9e 100644 --- a/src/features/CityTable/cityColumns.ts +++ b/src/features/CityTable/cityColumns.ts @@ -6,27 +6,11 @@ import { collatorFor, numberFormatFor } from "../../i18n/format"; import { resolveLocale } from "../../i18n/resolveLocale"; /** - * The columns the city table shows, in the order it shows them, built for one - * resolved locale. - * - * A builder rather than the module-scope array this used to be, because both - * halves of a column now follow the reader: the label comes out of the catalog, - * and the population cell is grouped by the tag's own rule rather than by - * whatever the machine running the code prefers. The collator goes in here too, - * fused into the default comparator at construction the way the accessor - * already is, which is what leaves the sort module and the table's own props - * untouched by any of this. - * - * The population formatting lives here because it is a fact about this column - * of this dataset, and the table body that renders it knows nothing about - * either. It goes through the cached formatter rather than the value's own - * per-call helper, which builds a formatter on every call and so builds one per - * rendered cell per render. - * - * The array this returns is a new identity every call, which is the whole point - * and also the hazard: the caller has to build it in a memo keyed on the - * catalog and the tag, or the sort and page memos downstream re-run on every - * render. + * The columns the city table shows, built for one resolved locale: both the + * label and the grouped population cell follow the reader, and the collator is + * fused into the default comparator here. A new identity every call, which is + * the hazard: the caller memoizes on the catalog and the tag, or the sort and + * page memos downstream re-run on every render. */ export function buildCityColumns(catalog: Catalog, tag: string) { const col = columns(collatorFor(tag)); @@ -47,41 +31,22 @@ export function buildCityColumns(catalog: Catalog, tag: string) { /** * One build at module scope, for the id union and the closed set below and for - * nothing else. Neither of those is a fact about a locale: which columns exist - * is the same in every language, and only what they are called moves. Deriving - * them from a build rather than declaring them beside it is what keeps a column - * that is added, renamed or removed from leaving a stale entry here. + * nothing else. Which columns exist is the same in every language; only what + * they are called moves. */ const BASE_COLUMNS = buildCityColumns(en, resolveLocale("en", []).tag); /** The literal union of the ids above, formed with no assertion anywhere. */ export type CityColumnId = (typeof BASE_COLUMNS)[number]["id"]; -/** - * The closed set a sort id restored from an address is checked for membership - * in. Derived from the columns rather than written out beside them, so a column - * that is added, renamed, or removed cannot leave a stale entry behind here. - */ +/** The closed set a restored sort id is checked against, derived not listed. */ export const CITY_COLUMN_IDS: readonly CityColumnId[] = BASE_COLUMNS.map( (column) => column.id, ); -/** - * The width every city id is padded to before it is handed over as a row - * identity. Ten because the dataset's ids are geoname ids, which the generator - * emits at ten digits, and the two rows the parse boundary numbers itself. A - * dataset whose ids outgrow this pads to no effect and the identities start - * ordering as text again, which is a visible reordering rather than a crash, so - * the constant is stated here rather than inlined. - */ +/** Ten, because the dataset's geoname ids are ten digits. */ const ID_WIDTH = 10; -/** - * A row's identity, as text, because that is what the table's tiebreak - * compares. Padded so identities that order as text order as the numbers they - * are: unpadded, "2" follows "1934976309" and the two lowest ids land at the - * end of every group of rows whose sorted column values are equal. City ids are - * unique by construction at the parse boundary, and padding preserves that. - */ +/** Text, because the tiebreak compares text. Padded so "2" precedes "19". */ export const cityRowId = (city: City) => String(city.id).padStart(ID_WIDTH, "0"); diff --git a/src/features/CityTable/cityLabels.ts b/src/features/CityTable/cityLabels.ts index 229625d..bbbe3c5 100644 --- a/src/features/CityTable/cityLabels.ts +++ b/src/features/CityTable/cityLabels.ts @@ -3,17 +3,9 @@ import type { SearchInputLabels } from "../../components/SearchInput"; import type { Catalog } from "../../i18n/catalogs/en"; /** - * The catalog, narrowed to the object the shared table takes. - * - * A catalog is a flat set of keys; the table's labels are a shape, with the page - * controls' own strings nested where the table hands them on. This is where the - * one becomes the other, and it is the only place the two vocabularies meet. - * - * The locale-sensitive entries take the resolved language tag ahead of the - * arguments the table supplies, and closing that tag in here is what keeps the - * table's own contract at the arity it has always had. An entry that needs no - * tag is passed through by reference rather than wrapped in an arrow that would - * only forward it, so the closure count stays at what the seam actually needs. + * The catalog, narrowed to the object the shared table takes: a flat set of + * keys becomes a shape. Closing the tag into the locale-sensitive entries here + * is what keeps the table's contract at the arity it has always had. */ export function buildTableLabels( catalog: Catalog, @@ -44,17 +36,7 @@ export function buildTableLabels( }; } -/** - * The catalog, narrowed to the two strings the search box shows. - * - * Its own builder rather than a third field on the object above, because the box - * is a sibling of the table rather than part of it: the table never sees these - * two strings and has no business carrying them. - * - * No tag, and the absence is the point. Neither entry weaves a number or a - * plural, so there is nothing here for a formatter to do, and taking a parameter - * that changes nothing would put a second key on the memo that reads it. - */ +/** The search box's own two strings. No tag: neither weaves a number. */ export function buildSearchLabels(catalog: Catalog): SearchInputLabels { return { name: catalog.searchName, diff --git a/src/features/CityTable/index.ts b/src/features/CityTable/index.ts index 40ad9d4..911a11c 100644 --- a/src/features/CityTable/index.ts +++ b/src/features/CityTable/index.ts @@ -1,7 +1,5 @@ export { CityTable } from "./CityTable"; -// The container above this feature needs the restored search term on its very -// first render, and this feature is what puts the term in the address. Passing -// the reader back out here keeps that dependency pointed at the feature rather -// than reaching past it into the shared table. +// The container needs the restored term on its first render, and this feature +// owns the address, so the reader is passed back out here. export { parseSearchTerm } from "../../components/DataTable/tableStateUrl"; diff --git a/src/features/Footer/Footer.tsx b/src/features/Footer/Footer.tsx index c5b1292..d2dab2a 100644 --- a/src/features/Footer/Footer.tsx +++ b/src/features/Footer/Footer.tsx @@ -1,33 +1,22 @@ import { useLocale } from "../../hooks/useLocale"; import styles from "./Footer.module.scss"; -// CC BY 4.0 asks for four things: credit the creator, link the source, link the -// licence, and say whether the work was changed. The catalog sentence is the -// whole of that obligation in every language, which is why it is unconditional -// and why Footer.test.tsx asserts it in more than one. Both modifications named -// there are corroborated by src/data/worldcities/license.txt and by what -// scripts/generate-cities.mjs does. -// -// The two names and the two addresses stay untranslated and stay here. They are -// identifiers rather than copy: the source calls itself this, and the licence -// identifier is what both a machine and a lawyer read. +// CC BY 4.0 asks for credit, a source link, a license link and a record of what +// changed. The catalog sentence carries all four in every language, which is why +// it is unconditional. The two names stay untranslated: they are identifiers. const SOURCE_NAME = "simplemaps.com World Cities"; const LICENSE_NAME = "CC BY 4.0"; -const LINK_URLS: Readonly> = { - [SOURCE_NAME]: "https://simplemaps.com/data/world-cities", - [LICENSE_NAME]: "https://creativecommons.org/licenses/by/4.0/", -}; +// A Map rather than an object literal: the key is a slice of catalog copy, and +// an object answers for every Object.prototype member as well as its own. +const LINK_URLS = new Map([ + [SOURCE_NAME, "https://simplemaps.com/data/world-cities"], + [LICENSE_NAME, "https://creativecommons.org/licenses/by/4.0/"], +]); -// Splits the sentence around the two identifiers, keeping each as a part of its -// own so it can be rendered as a link wherever the sentence put it. One catalog -// entry rather than the three fragments this would otherwise need: three would -// hold every language to English word order, and the pseudo-locale exists partly -// to make a sentence assembled out of several entries visible as several -// bracketed units. -// -// The only regular expression metacharacter in either name is a dot, which -// matches the dot it stands for. +// Splits the sentence around the two identifiers, so each can be a link +// wherever the sentence put it. One catalog entry rather than three fragments, +// which would hold every language to English word order. const EMBEDDED_NAMES = new RegExp(`(${SOURCE_NAME}|${LICENSE_NAME})`); export function Footer() { @@ -40,12 +29,10 @@ export function Footer() { .attribution(SOURCE_NAME, LICENSE_NAME) .split(EMBEDDED_NAMES) .map((part) => { - const href = LINK_URLS[part]; + const href = LINK_URLS.get(part); - // The part itself is the key. The split alternates run of text - // with identifier, so no two siblings carry the same string, and - // the position a part sits at is exactly what a translation is - // free to move. + // The part itself is the key: the split alternates text with + // identifier, and position is what a translation may move. return href === undefined ? ( part ) : ( diff --git a/src/features/Header/LocaleControl.tsx b/src/features/Header/LocaleControl.tsx index 47dcb3a..faa999b 100644 --- a/src/features/Header/LocaleControl.tsx +++ b/src/features/Header/LocaleControl.tsx @@ -6,27 +6,15 @@ import { CATALOG_IDS } from "../../i18n/resolveLocale"; import styles from "./LocaleControl.module.scss"; /** - * The language picker: following the machine, then one option per catalog that - * ships, each named in its own language. - * - * A native select rather than the segmented control its neighbour uses. Five - * options do not fit a row of buttons across a header bar, the page-size control - * in the table below is already a native select, and the native control brings - * keyboard handling, mobile behaviour and accessibility with nothing written by - * hand. - * - * The option list is built from the shipped ids rather than written out, so a - * catalog added later appears here without this file being edited. - * - * Its own two strings come from the catalog like every other word on the page. - * The autonyms below do not, and must not: a reader who cannot read the - * interface in front of them still has to find their own language in the list. + * The language picker: follow the machine, then one option per shipped catalog, + * each named in its own language. A native select rather than the segmented + * control beside it, because five options do not fit a row of buttons and the + * native control brings its keyboard and mobile behavior with it. */ export function LocaleControl() { const { catalog, choice, setChoice } = useLocale(); - // Document-global, so a constant here would give a second mounted control the - // same id and bind both labels to the first select. + // Document-global, so a constant would bind two labels to one select. const selectId = useId(); return ( diff --git a/src/features/Header/ThemeControl.tsx b/src/features/Header/ThemeControl.tsx index 00b3355..5bd1d2a 100644 --- a/src/features/Header/ThemeControl.tsx +++ b/src/features/Header/ThemeControl.tsx @@ -7,41 +7,28 @@ import type { ThemeChoice } from "../../theme/resolveTheme"; import styles from "./ThemeControl.module.scss"; /** - * The theme picker: three states, all visible at once, so choosing the operating - * system is as explicit as choosing a theme rather than being the absence of a - * choice. + * The theme picker: three states, all visible at once, so choosing the system + * is as explicit as choosing a theme. * - * a11y: every keyboard behaviour the radiogroup pattern calls for comes from the - * native inputs. Three radios sharing a name give arrow-key movement, wrap - * around, and one tab stop for the group, entered at the checked option. Nothing - * here handles a key, and nothing here should: the previous phase deliberately - * removed exactly this class of hand-written key handling from the sort header. - * - * This diverges from the pattern's non-native form in one place, deliberately: - * the checked state is left entirely to the inputs and no ARIA attribute - * restates it. Native radios already expose it through the property, and an - * attribute layered on top can only ever go stale against it. The prose here - * avoids naming that attribute so a search for it finds live code rather than a - * mention of it. + * a11y: every keyboard behavior the radiogroup pattern calls for comes from the + * native inputs. Nothing here handles a key, and nothing here should. The + * checked state is left to the inputs, because an ARIA attribute layered on top + * of the property can only ever go stale against it. */ export function ThemeControl() { const { choice, setChoice } = useTheme(); const { catalog } = useLocale(); - // The three states are offered in the order the theme vocabulary declares - // them, and each is named from the catalog. A record rather than a label - // beside each value, so the option list stays the one definition of what the - // states are and this is only how they are spelled. + // A record rather than a label beside each value, so the option list stays + // the one definition of what the states are. const names: Readonly> = { light: catalog.themeLight, dark: catalog.themeDark, system: catalog.themeSystem, }; - // The id and the radio name are both document-global, so writing either as a - // constant makes a second mounted control produce duplicate ids and put all - // six radios in one group, where every label binds to the first matching - // input. Generated per instance, the two controls cannot reach each other. + // Document-global, so a constant would put a second mounted control's radios + // in this group and bind its labels to these inputs. const groupName = useId(); return ( diff --git a/src/features/RootLayout/ErrorBoundary.tsx b/src/features/RootLayout/ErrorBoundary.tsx index 40ca136..da3c68c 100644 --- a/src/features/RootLayout/ErrorBoundary.tsx +++ b/src/features/RootLayout/ErrorBoundary.tsx @@ -5,17 +5,9 @@ import styles from "./RootLayout.module.css"; import type { ReactNode } from "react"; /** - * The fallback's two strings. - * - * They arrive as props rather than being read from the catalog here, and the - * reason is the class: this is the only render-fallback mechanism React offers - * and it can only be a class component, so it cannot call a hook. Its parent is - * the locale subscriber and hands the copy down. - * - * The fallback shows this authored copy and nothing else. A render-time throw's - * own message is whatever the engine produced, so it would be noise on screen - * and mild information disclosure, and the surrounding sentence would have to - * work as a frame around an arbitrary string. + * The fallback's two strings, as props because a class cannot call a hook. The + * fallback shows this authored copy and never the throw's own message, which is + * engine text and mild information disclosure. */ export interface ErrorBoundaryLabels { readonly message: string; @@ -31,10 +23,8 @@ interface ErrorBoundaryState { hasError: boolean; } -// The only render-fallback mechanism React offers. The root error callbacks -// added in React 19 report a caught error but render nothing, so they are not -// an alternative to this. Boundaries also cannot catch a promise rejection, -// which is why the table keeps its own inline error region beside this one. +// The only render-fallback mechanism React offers. It cannot catch a promise +// rejection, which is why the table keeps its own inline error region. export class ErrorBoundary extends Component< ErrorBoundaryProps, ErrorBoundaryState @@ -47,9 +37,8 @@ export class ErrorBoundary extends Component< // The reporting lifecycle method is omitted; a client-only bundle has nowhere to send a report. - // Resetting the boundary's own state re-renders the children. A document - // reload would throw away the fetched dataset and re-download roughly three - // megabytes to recover from what is most likely a render-local fault. + // Resetting state re-renders the children. A reload would re-download the + // whole dataset to recover from a probably render-local fault. handleReset = () => { this.setState({ hasError: false }); }; diff --git a/src/features/RootLayout/RootLayout.tsx b/src/features/RootLayout/RootLayout.tsx index f148430..5cb4b6d 100644 --- a/src/features/RootLayout/RootLayout.tsx +++ b/src/features/RootLayout/RootLayout.tsx @@ -11,10 +11,8 @@ interface RootLayoutProps { } export function RootLayout({ children }: RootLayoutProps) { - // The layout subscribes to the locale on the boundary's behalf. A class - // component cannot call a hook, and the boundary below has to be a class - // because that is the only render-fallback mechanism React offers, so its two - // strings are read here and handed down. + // Subscribed here on the boundary's behalf: a class cannot call a hook, and + // the boundary has to be a class. const { catalog } = useLocale(); return ( diff --git a/src/hooks/useDebouncedCallback.ts b/src/hooks/useDebouncedCallback.ts index cd48ece..89be848 100644 --- a/src/hooks/useDebouncedCallback.ts +++ b/src/hooks/useDebouncedCallback.ts @@ -1,44 +1,12 @@ import { useCallback, useEffect, useMemo, useRef } from "react"; -/** - * A debounced call and the means to drop one that is already pending. - * - * Two named members rather than a cancel hung on the scheduler: attaching one - * means mutating a memoized function during render, which is the shape this - * project's lint gate rejects. Both members are stable, so a caller that - * destructures the pair can put either in a dependency array; the wrapper is - * memoized too, for a caller that holds it whole. - */ +/** A debounced call, plus the means to drop one that is already pending. */ export interface DebouncedCallback { readonly schedule: (...args: A) => void; readonly cancel: () => void; } -/** - * Debounces a call rather than a value: the scheduler it returns schedules the - * callback and reschedules it on every further call, so a burst of calls - * settles into one invocation carrying the arguments of the last. - * - * Debouncing the call is what keeps this usable from an event handler. A - * debounced value has to be turned into a state write or a callback somewhere, - * and the only place left for that is an effect, which is the shape this - * project's lint gate now rejects. Scheduling inside the handler makes the - * commit an ordinary event-driven write instead. - * - * The pending handle lives in a ref and is cleared on unmount, so a call still - * in flight when the component tears down never lands. The cancel covers the - * case that is not a teardown: a caller that has just replaced the state a - * pending call was made against needs to drop that call, and without a cancel - * its only alternative is to let the stale value land on top of the new one. - * - * The callback is read out of a ref when the timer fires rather than closed - * over when the call was scheduled, so a pending call always runs the current - * implementation instead of the one that happened to be current a moment - * earlier. That is also what lets the scheduler be memoized over the delay - * alone: it holds one identity for the life of the component whatever the - * caller does with its own, so an inline arrow is as safe here as a memoized - * callback rather than being a stale-closure trap the types cannot catch. - */ +/** Debounces a call, reading the callback from a ref when the timer fires. */ export function useDebouncedCallback( callback: (...args: A) => void, delay: number, @@ -63,10 +31,8 @@ export function useDebouncedCallback( (...args: A) => { clearTimeout(pending.current); pending.current = setTimeout(() => { - // Dropped as it fires, for the same reason the cancel below drops it: - // a handle the platform has already retired must not be clearable a - // second time, and a call that has landed retires its own handle just - // as surely as a cancel does. + // Dropped as it fires: a handle the platform has already retired must + // not be clearable a second time. pending.current = undefined; latest.current(...args); }, delay); @@ -74,9 +40,8 @@ export function useDebouncedCallback( [delay], ); - // The handle is dropped as well as cleared, so a cancel followed by another - // cancel clears nothing rather than an identifier the platform has since - // handed to someone else's timer. + // The handle is dropped as well as cleared, so a second cancel clears nothing + // rather than an identifier the platform has since reissued. const cancel = useCallback(() => { clearTimeout(pending.current); pending.current = undefined; diff --git a/src/hooks/useLocale.ts b/src/hooks/useLocale.ts index ce1b40d..024f183 100644 --- a/src/hooks/useLocale.ts +++ b/src/hooks/useLocale.ts @@ -8,39 +8,22 @@ import { subscribeLocale, } from "../i18n/localeStore"; -/** - * Owns the reader's locale for one component: the choice, the catalog its - * strings come from, the tag the platform formatters take, and the two - * attributes the document element carries. - * - * Many instances by construction, which is the opposite of the theme hook - * beside it and worth stating rather than inheriting. That hook holds its choice - * per caller, so two callers hold two choices and their two effects race on one - * document element. This one holds no state at all: the choice lives in a single - * module-scope store, so every subscriber resolves the identical value and the - * duplicate writes below agree by construction rather than by there being only - * one of them. - * - * The language attribute takes the resolved tag rather than the catalog id. The - * tag is the field that is a well formed language tag for every catalog, the - * pseudo-locale's strings really are English, and it keeps the attribute naming - * the same locale the platform formatters are given. - */ +/** Many instances by construction, because it holds no state of its own. */ export function useLocale() { const locale = useSyncExternalStore(subscribeLocale, getLocaleSnapshot); const choice = useSyncExternalStore(subscribeLocale, getChoiceSnapshot); useEffect(() => { - // Both, because the inline script sets both before first paint and the one - // left unmaintained goes stale the first time the reader chooses. + // Both, because the inline script sets both before first paint and either + // one left unmaintained goes stale the first time the reader chooses. document.documentElement.lang = locale.tag; document.documentElement.dir = locale.dir; }, [locale]); return { choice, - // The store's own setter, which already holds one identity for the life of - // the document, so nothing here has to memoize it back into one. + // The store's own setter, already one identity for the life of the + // document, so nothing here has to memoize it back into one. setChoice: setLocaleChoice, catalog: CATALOGS[locale.catalog], tag: locale.tag, diff --git a/src/hooks/usePaginatedRows.ts b/src/hooks/usePaginatedRows.ts index 4d9c28a..a05b41d 100644 --- a/src/hooks/usePaginatedRows.ts +++ b/src/hooks/usePaginatedRows.ts @@ -2,14 +2,7 @@ import { useMemo } from "react"; import { paginate, type PaginateResult } from "../components/paginate"; -/** - * Memoizes one page of rows. - * - * The page position it reports back has been clamped to the number of pages - * that exist, which is a value to render and never one to write back into - * state: storing it would strand the user on whatever a narrowed result set - * allowed rather than restoring them when it widens again. - */ +/** Memoizes one page. The clamped position is to render, never to store. */ export function usePaginatedRows( rows: readonly T[], page: number, diff --git a/src/hooks/useSortedRows.ts b/src/hooks/useSortedRows.ts index 293f9f1..883b5a9 100644 --- a/src/hooks/useSortedRows.ts +++ b/src/hooks/useSortedRows.ts @@ -3,15 +3,7 @@ import { useMemo } from "react"; import type { Column } from "../components/DataTable/column"; import { sortRows } from "../components/DataTable/sortRows"; -/** - * Memoizes the sorted rows, resolving the active column id to its descriptor - * inside the memo so the caller passes an id rather than a descriptor it would - * otherwise have to look up and keep stable itself. - * - * Every argument is a dependency, so a caller that rebuilds the column array or - * the identity function on each render defeats the memo and re-sorts the whole - * collection on every keystroke. Both belong at module scope. - */ +/** Memoizes the sorted rows. A column array rebuilt each render re-sorts. */ export function useSortedRows( rows: readonly T[], columns: readonly Column[], diff --git a/src/hooks/useTheme.ts b/src/hooks/useTheme.ts index e2fdf98..6e817bd 100644 --- a/src/hooks/useTheme.ts +++ b/src/hooks/useTheme.ts @@ -8,23 +8,7 @@ import { } from "../theme/resolveTheme"; import type { ThemeChoice } from "../theme/resolveTheme"; -/** - * Reads the stored choice. Anything that is not exactly one of the two explicit - * words is absent, and so is a store that cannot be read at all: a stale entry - * from an older build and a hostile one are the same case, and both render the - * default rather than an undefined theme. - * - * Membership is tested against the declared vocabulary rather than against two - * words written out again here, so the accepted set has one definition. The - * default word is excluded on its own line: the key holding it is already - * treated as absent, because the default has exactly one representation and it - * is the key not being there. - * - * The property access is what throws when site data is blocked, so neither a - * typeof guard nor optional chaining substitutes for the catch. Unguarded, the - * throw happens inside a state initializer, which unmounts the whole tree over a - * display preference. - */ +/** Anything but the two explicit words is absent. The access can throw. */ function readStoredChoice(): ThemeChoice { try { const stored = localStorage.getItem(THEME_STORAGE_KEY); @@ -42,20 +26,12 @@ function readStoredChoice(): ThemeChoice { return "system"; } -/** - * Whether this environment can answer a media query at all. Asked in one place - * rather than two: the reader below and the subscription below that have to - * agree, or the hook subscribes to something it will not read. - */ +/** Asked once, or the reader and the subscription below could disagree. */ function supportsMediaQueries(): boolean { return typeof window.matchMedia === "function"; } -/** - * The operating system's current preference. An environment with no media query - * support prefers light, which is the same answer the resolver gives for a - * missing choice, so nothing downstream has a third case to handle. - */ +/** The system preference. No media query support prefers light. */ function readPrefersDark(): boolean { if (!supportsMediaQueries()) { return false; @@ -64,18 +40,7 @@ function readPrefersDark(): boolean { return window.matchMedia(PREFERS_DARK_QUERY).matches; } -/** - * Subscribes to the operating system's preference, returning the unsubscribe. - * - * Paired with the reader above through useSyncExternalStore rather than through - * an effect that seeds state and then re-reads it. The preference can move - * between the render that would seed it and the commit that would subscribe, - * and StrictMode's mount, unmount, remount opens that window a second time; the - * hook closes both by reading the store itself after subscribing. - * - * An environment with no media query support subscribes to nothing and keeps - * the reader's answer, so the two cannot disagree about whether a query exists. - */ +/** Through useSyncExternalStore, not an effect, which leaves a window. */ function subscribePrefersDark(onStoreChange: () => void): () => void { if (!supportsMediaQueries()) { return () => {}; @@ -90,21 +55,16 @@ function subscribePrefersDark(onStoreChange: () => void): () => void { } /** - * Owns the theme: the stored choice, the write-through when it changes, the - * concrete theme stamped on the document element, and the two subscriptions that - * keep it current, one to the operating system and one to the other tabs. + * Owns the theme: the stored choice, the write-through, the theme stamped on + * the document element, and the subscriptions to the system and the other tabs. * - * Single instance by construction. The choice is per caller, the document - * element the effect stamps it onto is not, so a second caller gets its own - * choice and the two effects race on every render, leaving the loser showing a - * control that disagrees with the painted page. Lift this behind a provider - * mounted once before the second caller exists. + * Single instance by construction. The choice is per caller and the document + * element is not, so two callers hold two choices and their two effects race on + * one element. Lift this behind a provider before adding a second caller. * - * The rule that turns a choice plus a preference into a theme is written a - * second time, as a literal, inside the blocking inline script in index.html. - * That script runs before any module loads, so it cannot import this. A change - * to either one needs the same change to the other, and the parity guard in - * src/toolchain.test.ts is what fails when they stop agreeing. + * The choice-plus-preference rule is written a second time as a literal in the + * blocking inline script in index.html, which cannot import a module. Change + * both together; the parity guard in src/toolchain.test.ts holds them. */ export function useTheme() { const [choice, setChoiceState] = useState(readStoredChoice); @@ -115,10 +75,8 @@ export function useTheme() { useEffect(() => { const handleStorageChange = (event: StorageEvent) => { - // This key only, and only a write from another document: the event does - // not fire in the tab that made it, which is why the setter below does - // not have to guard against reacting to itself. A null key is a clear() - // rather than a write, and it takes this key with it. + // This key only. The event does not fire in the tab that wrote it, so the + // setter below need not guard against itself. A null key is a clear(). if (event.key === null || event.key === THEME_STORAGE_KEY) { setChoiceState(readStoredChoice()); } @@ -135,7 +93,7 @@ export function useTheme() { useEffect(() => { // Both halves, because the inline script sets both before first paint and - // the one left unmaintained goes stale the first time the user chooses. + // either one left unmaintained goes stale the first time the reader picks. document.documentElement.dataset.theme = resolved; document.documentElement.style.colorScheme = resolved; }, [resolved]); @@ -145,8 +103,8 @@ export function useTheme() { try { if (next === "system") { - // A delete, not the word: the default has exactly one representation, - // and every other value the key could hold is already treated as absent. + // A delete, not the word: the default has one representation, the key + // not being there. localStorage.removeItem(THEME_STORAGE_KEY); } else { localStorage.setItem(THEME_STORAGE_KEY, next); diff --git a/src/i18n/catalogs/en.ts b/src/i18n/catalogs/en.ts index da28c84..9d1e606 100644 --- a/src/i18n/catalogs/en.ts +++ b/src/i18n/catalogs/en.ts @@ -2,60 +2,25 @@ import type { DatasetErrorCode } from "../../api/getCities"; import { numberFormatFor, selectPlural } from "../format"; /** - * The nouns the two woven sentences below pluralize, one record per noun, - * total over the two categories the English tag reports. - * - * Total rather than defaulted, so there is no fallback arm here that nothing - * can reach and no branch the coverage gate cannot cover. The category set is - * CLDR data rather than a type, so what holds these honest is the catalog test, - * which calls every catalog with a count drawn from each of its own tag's - * categories and reads the result for a hole. + * The nouns the woven sentences pluralize, total over the two categories + * English reports, so there is no branch the coverage gate cannot cover. */ const CITY = { one: "city", other: "cities" }; const RESULT = { one: "result", other: "results" }; const ENTRY = { one: "entry", other: "entries" }; -/** - * Which way a sorted column runs. - * - * Declared here rather than imported from the table, because a catalog names - * words and must not learn what a table is. It is the same pair of tokens the - * sort state travels as, and it arrives at the two entries below as a value - * precisely so that no sentence has to build a word out of it. - */ +/** Declared here rather than imported: a catalog must not learn about tables. */ export type SortedDirection = "asc" | "desc"; -/** - * The two directions, as the words the sentences below weave in. - * - * A record rather than a suffix appended to the token, and the difference is - * the whole reason this file changed: "ascend" plus "ing" is a word in exactly - * one language, and a sentence assembled that way cannot be translated at all. - * Every language spells the pair out here and reads it by key. - */ +/** A record rather than a suffix: "ascend" plus "ing" is one language only. */ const DIRECTION: Readonly> = { asc: "ascending", desc: "descending", }; /** - * What a reader is told when the city data cannot be loaded, one sentence per - * failure code. - * - * Total over the code union rather than defaulted, so a code added to the loader - * without a sentence in all four catalogs fails the type check instead of - * rendering the word undefined at the moment the application has already - * failed. There is no fallback arm here and so no branch the coverage gate - * cannot reach. - * - * Every entry has the same signature and most ignore both arguments, which is - * what keeps the lookup one call with no branch. The three that use the second - * one weave a row index or a response status and group it on the resolved tag, - * like every other number this application shows. - * - * The field count in the two row sentences is written out rather than passed - * in. It is a fact about the asset's shape rather than a quantity a reader's - * locale groups, and the detail slot is already carrying the row. + * One sentence per failure code, total over the code union, so a new code fails + * the type check and no branch is left the coverage gate cannot reach. */ export type DatasetErrorText = Readonly< Record string> @@ -78,21 +43,7 @@ const DATASET_ERROR_TEXT: DatasetErrorText = { unexpected: () => "An unexpected error occurred.", }; -/** - * The base catalog: every string the city table shows that names what its rows - * are or what a column of them holds, in the language the rest of the tree is - * checked against. - * - * The wording is the wording the table already shipped, up to the two nouns - * that now follow their count and the two counts that are now grouped. - * - * The entries taking the resolved language tag as their first parameter take it - * for one reason: the count a reader sees is grouped by that tag's own rule, and - * the noun beside it is selected over the categories the tag reports. Neither - * decision can be made where the sentence is assembled, because that is one - * layer below the locale by construction. An entry needing neither takes no tag, - * so the signature says which entries are locale-sensitive and which are copy. - */ +/** An entry takes the tag only when it groups a number or picks a plural. */ export const en = { appTitle: "City List", themeGroup: "Theme", @@ -145,10 +96,5 @@ export const en = { }, }; -/** - * The shape every other catalog is held to. Derived from the base rather than - * declared beside it, so the key set has one definition: a catalog missing a key - * or misspelling one fails the type check rather than rendering undefined at a - * reader. - */ +/** Derived from the base, so a missing key in another catalog fails to type. */ export type Catalog = typeof en; diff --git a/src/i18n/catalogs/es.ts b/src/i18n/catalogs/es.ts index b67f4bc..376b762 100644 --- a/src/i18n/catalogs/es.ts +++ b/src/i18n/catalogs/es.ts @@ -2,14 +2,8 @@ import { numberFormatFor, selectPlural } from "../format"; import type { Catalog, DatasetErrorText, SortedDirection } from "./en"; /** - * The nouns the two woven sentences below pluralize, total over the three - * categories the Spanish tag reports. - * - * The many form is spelled out rather than shared with the other form, even - * though Spanish inflects them the same way. The category exists because - * Spanish treats round millions differently in compact notation, and writing - * the arm out is what makes the record total by construction rather than by a - * reader remembering that two of the three happen to agree today. + * Total over the three categories Spanish reports. The many form is spelled + * out, which makes the record total by construction. */ const CIUDAD = { one: "ciudad", many: "ciudades", other: "ciudades" }; const RESULTADO = { one: "resultado", many: "resultados", other: "resultados" }; @@ -41,13 +35,7 @@ const TEXTO_DE_ERROR: DatasetErrorText = { unexpected: () => "Se produjo un error inesperado.", }; -/** - * The Spanish catalog. - * - * Declared with satisfies rather than annotated with it, so a missing key and a - * misspelled key are both compile errors while the literal types of the entries - * survive for anything that wants to read them. - */ +/** Declared with satisfies, so a missing key is a compile error. */ export const es = { appTitle: "Lista de ciudades", themeGroup: "Tema", diff --git a/src/i18n/catalogs/fr.ts b/src/i18n/catalogs/fr.ts index 1a44c9f..ec183e3 100644 --- a/src/i18n/catalogs/fr.ts +++ b/src/i18n/catalogs/fr.ts @@ -1,15 +1,7 @@ import { numberFormatFor, selectPlural } from "../format"; import type { Catalog, DatasetErrorText, SortedDirection } from "./en"; -/** - * The nouns the two woven sentences below pluralize, total over the three - * categories the French tag reports. - * - * French puts zero in the singular category where English and Spanish put it in - * the plural, so "0 ville" is correct here and "0 ciudades" is correct beside - * it. That is the rule a ternary on the count gets wrong without ever looking - * wrong, and it is the reason the selection goes through the platform. - */ +/** Total over three categories. French puts zero in the singular. */ const VILLE = { one: "ville", many: "villes", other: "villes" }; const RESULTAT = { one: "résultat", many: "résultats", other: "résultats" }; const ENTREE = { one: "entrée", many: "entrées", other: "entrées" }; @@ -20,17 +12,7 @@ const ORDRE: Readonly> = { desc: "décroissant", }; -/** - * The narrow no-break space French typography sets before a colon, a semicolon, - * an exclamation mark and a question mark. - * - * Written as an escape rather than as the character, because the character is - * indistinguishable from an ordinary space in every editor and terminal this - * file is read in, and a reviewer meeting it inline would correct it. It is a - * translation requirement, not a typo. The same character is already in every - * grouped number these entries carry, put there by the formatter rather than by - * hand. - */ +/** U+202F, as an escape: inline it is indistinguishable from a space. */ const NARROW_NO_BREAK_SPACE = "\u202F"; /** Ce qui est annoncé au lecteur quand les données ne peuvent pas être chargées. */ @@ -52,20 +34,7 @@ const TEXTE_ERREUR: DatasetErrorText = { unexpected: () => "Une erreur inattendue s'est produite.", }; -/** - * The French catalog. - * - * Declared with satisfies rather than annotated with it, so a missing key and a - * misspelled key are both compile errors while the literal types of the entries - * survive for anything that wants to read them. - * - * French typography puts a narrow no-break space, U+202F, before a colon, a - * semicolon, an exclamation mark and a question mark. Two entries below carry - * one: the label above the page-size control and the prefix on a failure. Both - * reach it through the named constant above rather than by holding the - * character inline, so a reviewer reads the requirement instead of a space that - * looks like a typo. - */ +/** Declared with satisfies, so a missing key is a compile error. */ export const fr = { appTitle: "Liste des villes", themeGroup: "Thème", diff --git a/src/i18n/catalogs/index.ts b/src/i18n/catalogs/index.ts index d85476e..eccf495 100644 --- a/src/i18n/catalogs/index.ts +++ b/src/i18n/catalogs/index.ts @@ -4,13 +4,7 @@ import { es } from "./es"; import { fr } from "./fr"; import { pseudo } from "./pseudo"; -/** - * Every catalog, by id. - * - * Total over the closed union rather than looked up with a fallback, so a value - * that somehow reached here unchecked still cannot find a missing arm, and a - * catalog added later cannot be forgotten here without failing the type check. - */ +/** Every catalog, by id, and total, so a new one cannot be forgotten here. */ export const CATALOGS: Readonly> = { en, es, @@ -18,12 +12,7 @@ export const CATALOGS: Readonly> = { "ar-XB": pseudo, }; -/** - * What each catalog calls itself, written in its own language and never - * translated. A reader who cannot read the interface they are looking at has to - * be able to find their own language in the picker, which is the whole job of - * this record and the reason there is exactly one literal per id. - */ +/** What each catalog calls itself, in its own language, never translated. */ export const AUTONYMS: Readonly> = { en: "English", es: "Español", diff --git a/src/i18n/catalogs/pseudo.ts b/src/i18n/catalogs/pseudo.ts index d84797d..25a146c 100644 --- a/src/i18n/catalogs/pseudo.ts +++ b/src/i18n/catalogs/pseudo.ts @@ -5,39 +5,9 @@ import { type SortedDirection, } from "./en"; -/** - * The right-to-left pseudo-locale. - * - * It exists so the direction and the truncation can be tested, because the - * three catalogs beside it all read left to right and would leave both - * untestable. - * - * A deliberate hybrid of the two pseudo-locales the browsers already ship: the - * direction comes from the right-to-left one, the readability and the padding - * come from the left-to-right one. The character reversal the real right-to-left - * pseudo-locale performs is dropped on purpose: anyone reading this repository - * has to be able to read this catalog, which is why it was taken over shipping - * Arabic strings nobody here can review. - * - * Every entry is derived from the corresponding entry of the base catalog rather - * than committed as a transformed literal, so this file cannot drift from the - * copy it pseudo-translates. - * - * The two bidirectional control characters this file needs are written as - * escapes rather than as glyphs. They are invisible either way, and a raw one - * in source is the shape a hidden-character attack takes, so tooling flags it - * and is right to. The escape says the same thing in characters a reviewer can - * see. - */ +// The pseudo-locale, so direction and truncation have something to prove. -/** - * Opens a run whose direction is taken from its first strongly directional - * character, so a Latin string renders as its own left-to-right run inside a - * right-to-left document instead of having its punctuation scattered. - * - * An isolate rather than a directional mark, because a mark states a direction - * at a point and cannot bound a run. - */ +/** An isolate, not a mark: a mark cannot bound a run, only start one. */ const FIRST_STRONG_ISOLATE = "\u2066"; /** Closes the run the isolate above opened. */ @@ -46,30 +16,14 @@ const POP_DIRECTIONAL_ISOLATE = "\u2069"; /** What the padding is made of. Visibly filler, so nobody reads it as copy. */ const PADDING_CHARACTER = "~"; -/** - * One message, pseudo-translated. - * - * Three things at once, each catching a different defect. The brackets bound the - * message unit, so a sentence assembled out of two catalog entries shows up as - * two bracketed units rather than as one plausible line. The isolates keep the - * readable run readable inside a right-to-left document. The padding grows the - * string by roughly a third, which is about what a real translation costs, so a - * layout that truncates or overflows does it here rather than in front of a - * reader. - */ +/** The brackets bound the unit; the padding is what a translation costs. */ export function pseudoize(message: string): string { const padding = PADDING_CHARACTER.repeat(Math.ceil(message.length / 3)); return `[${FIRST_STRONG_ISOLATE}${message}${POP_DIRECTIONAL_ISOLATE} ${padding}]`; } -/** - * The dataset failure sentences, each derived from the base catalog's own so - * this record cannot drift from the copy it pseudo-translates. Written out - * entry by entry like every other entry in this file, rather than built from - * the code tuple, because a construction would need a cast to be typed and the - * cast is what would hide a missing arm. - */ +/** Entry by entry, because a built record would need a cast to be typed. */ const DATASET_ERROR_TEXT: DatasetErrorText = { notAnObject: (tag, detail) => pseudoize(en.datasetError.notAnObject(tag, detail)), @@ -87,16 +41,7 @@ const DATASET_ERROR_TEXT: DatasetErrorText = { pseudoize(en.datasetError.unexpected(tag, detail)), }; -/** - * The pseudo-locale catalog. Every function-valued entry pseudo-translates the - * base catalog's result rather than a template, so the values woven into a - * sentence land inside the brackets where a truncation would cut them. - * - * It declares no plural nouns of its own, and that is not an omission. Its - * strings really are English and its resolved tag really is the English one, so - * the base catalog's two categories are its two categories, and a second set - * here could only ever drift from them. - */ +/** Each entry translates the base result, so woven values stay bracketed. */ export const pseudo = { appTitle: pseudoize(en.appTitle), themeGroup: pseudoize(en.themeGroup), diff --git a/src/i18n/datasetErrorText.ts b/src/i18n/datasetErrorText.ts index ee40e8e..f5037d5 100644 --- a/src/i18n/datasetErrorText.ts +++ b/src/i18n/datasetErrorText.ts @@ -3,22 +3,9 @@ import { DatasetError } from "../api/getCities"; import type { Catalog } from "./catalogs/en"; /** - * The sentence a reader is shown for a failed load. - * - * Called during render rather than where the failure is caught, and the - * difference matters: the catch lives inside the container's fetch effect, so - * reading the catalog there would put the locale in that effect's dependency - * array and changing the language would re-issue the search. - * - * The lookup is total over a closed union of codes read off a class instance, - * so nothing a reader controls indexes it and there is no fallback arm inside - * it. Only the selected sentence is returned; the failure's own message is - * developer-facing text and its preserved cause is engine text, and neither is - * something a reader was ever meant to see. - * - * A failure that is not a dataset error is one this application has no code - * for, and it takes the unexpected sentence. That is the second of the two - * branches here, and it is the one a rejection carrying no error at all reaches. + * The sentence a reader is shown for a failed load. Called during render, not + * at the catch, which would put the locale in the fetch effect's dependencies. + * The second of the two branches is the one a rejection with no error reaches. */ export function datasetErrorText( error: Error, diff --git a/src/i18n/format.ts b/src/i18n/format.ts index c551cd5..6c90fe6 100644 --- a/src/i18n/format.ts +++ b/src/i18n/format.ts @@ -1,35 +1,16 @@ -// The one module in the tree that asks the platform for a locale. Every -// locale-sensitive decision the application makes reaches a constructor here, -// keyed by the resolved tag, and a guard in src/toolchain.test.ts fails if a -// second module grows one of its own. +// The one module in the tree that asks the platform for a locale. A guard in +// src/toolchain.test.ts fails if a second module grows a constructor of its own. /** - * One instance per tag, held for the module's lifetime. - * - * The reasoning moved here from the single module-scope collator this replaces, - * and it survives the move intact: building a collator inside the comparison - * would build roughly eight hundred thousand of them for a single sort of the - * full dataset. The value formatter has the same shape of cost one layer up, - * because the platform's per-value formatting helper builds a formatter on - * every call, which is one per rendered cell per render. - * - * What changed is that the instance can no longer be a constant: it is a - * function of the resolved tag, and the tag moves when the reader chooses. A - * map keyed by tag is what keeps one instance per locale rather than one - * instance per document, and it is a cache with a known ceiling rather than one - * that grows with input, because the only keys reaching it are the tags of a - * four-entry frozen record. + * One instance per tag, held for the module's lifetime. A collator built inside + * a comparison would be roughly eight hundred thousand of them for one sort of + * the full dataset. Keyed by tag, so the ceiling is the catalog count. */ const collators = new Map(); const numberFormats = new Map(); const pluralRules = new Map(); -/** - * The one lookup the three below share, written once rather than three times. - * Absence is tested rather than truthiness because the value is an object and - * the map holds no falsy ones, so the two would agree today and diverge the - * first time something cacheable is not. - */ +/** The one lookup the three below share. Absence, not truthiness. */ function cached( cache: Map, tag: string, @@ -60,24 +41,7 @@ export function pluralRulesFor(tag: string): Intl.PluralRules { return cached(pluralRules, tag, (forTag) => new Intl.PluralRules(forTag)); } -/** - * Picks a noun's form for a count, over the categories the tag itself reports - * rather than over a singular-or-other pair. - * - * The pair is wrong in half the catalogs that ship: Spanish and French each - * report three categories where English reports two, and French puts zero in - * the singular where the other two put it in the plural. A ternary on the count - * cannot express either of those, and would be silently wrong rather than - * visibly missing. - * - * The assertion on the selected category is the one place in this module where - * the platform's answer is narrowed to what the caller declared, and it is - * sound exactly while a caller's record is total over the categories its own - * tag reports. Nothing in the type system can check that, because the category - * set is CLDR data rather than a type, so the catalog test asserts it directly: - * every catalog is called with a count drawn from each of its tag's categories - * and its sentences are read for a hole. - */ +/** Over the categories the tag reports, not a singular-or-other pair. */ export function selectPlural( tag: string, count: number, diff --git a/src/i18n/localeStore.ts b/src/i18n/localeStore.ts index beb529c..b807246 100644 --- a/src/i18n/localeStore.ts +++ b/src/i18n/localeStore.ts @@ -7,26 +7,10 @@ import { type ResolvedLocale, } from "./resolveLocale"; -/** - * The chosen locale, owned once for the whole document. - * - * The theme's choice lives inside its hook, one copy per caller, and its own - * documentation says that is why a second caller would race the first. This one - * has several callers by design: the picker in the header and the table below - * it both read it, and both stamp the same document element. Holding the choice - * here rather than in the hook is what makes those writes agree by - * construction, because every subscriber resolves the identical value. - */ +// The chosen locale, owned once for the document, which is what makes several +// callers agree rather than each resolving one of its own. -/** - * Reads the stored choice. Anything that is not the id of a catalog that ships - * is absent, and so is a store that cannot be read at all: a stale entry from an - * older build and a hostile one are the same case, and both resolve to - * following the machine rather than to an undefined locale. - * - * The property access is what throws when site data is blocked, so neither a - * typeof guard nor optional chaining substitutes for the catch. - */ +/** Anything not a shipped id is absent. The access throws when blocked. */ function readStoredChoice(): LocaleChoice { try { const stored = localStorage.getItem(LOCALE_STORAGE_KEY); @@ -52,15 +36,7 @@ function notify(): void { } } -/** - * Another document wrote the key. Re-read the store rather than trusting the - * value the event carries, and ignore every other key: the event does not fire - * in the document that made the write, which is why the setter below does not - * have to guard against reacting to itself. - * - * A null key is a clear() rather than a write, and it takes this key with it, so - * it counts the same as a write to this key. - */ +/** Re-read rather than trust the event. A null key is a clear(). */ function handleStorage(event: StorageEvent): void { if (event.key === null || event.key === LOCALE_STORAGE_KEY) { choice = readStoredChoice(); @@ -69,14 +45,9 @@ function handleStorage(event: StorageEvent): void { } /** - * Registers a reader and returns the unregister. - * - * The window listeners are installed with the first subscriber and removed with - * the last, so a document with nothing mounted holds nothing. That is also why - * the choice is re-read here: between the last unsubscribe and this call there - * was no listener, so a write from another document in that window went unseen. - * React re-reads the snapshot immediately after subscribing, which is what turns - * the re-read into a render rather than into a value nobody asked for. + * The listeners come and go with the first and last subscriber, so the choice + * is re-read here: nothing was listening between the last unsubscribe and this + * call, and a cross-tab write in that window went unseen. */ export function subscribeLocale(onStoreChange: () => void): () => void { if (subscribers.size === 0) { @@ -97,14 +68,7 @@ export function subscribeLocale(onStoreChange: () => void): () => void { }; } -/** - * The resolved locale right now. - * - * Safe to hand straight to React with no cache in front of it, because the - * resolver returns one of four module constants and so its identity is already - * stable for a stable input. A resolver that built its answer would need one, - * and the loop it would otherwise cause is the reason this is worth stating. - */ +/** Safe to hand straight to React: the resolver returns a module constant. */ export function getLocaleSnapshot(): ResolvedLocale { return resolveLocale(choice, navigator.languages); } @@ -115,13 +79,8 @@ export function getChoiceSnapshot(): LocaleChoice { } /** - * Moves the choice and writes it through. - * - * Takes a string rather than the union because the picker hands over whatever - * the DOM has in the control's value, and this is the one place that decides - * whether a value names a choice. A value that names none is ignored outright: - * the option list is closed, so nothing that reaches this branch came from a - * reader picking something. + * Moves the choice and writes it through. A value naming none is ignored: the + * option list is closed, so nothing reaching that branch came from a reader. */ export function setLocaleChoice(next: string): void { if (!isLocaleChoice(next)) { @@ -132,8 +91,8 @@ export function setLocaleChoice(next: string): void { try { if (next === "system") { - // A delete, not the word: the default has exactly one representation, and - // every other value the key could hold is already treated as absent. + // A delete, not the word: the default has one representation, the key + // not being there. localStorage.removeItem(LOCALE_STORAGE_KEY); } else { localStorage.setItem(LOCALE_STORAGE_KEY, next); diff --git a/src/i18n/resolveLocale.ts b/src/i18n/resolveLocale.ts index ab6a707..2ffe348 100644 --- a/src/i18n/resolveLocale.ts +++ b/src/i18n/resolveLocale.ts @@ -1,52 +1,28 @@ -// The locale's vocabulary and the one rule that turns a stored choice plus the -// reader's own preferences into something the document, the catalogs and the -// platform formatters can each act on. Everything the inline script in +// The locale's vocabulary and the rule that turns a stored choice plus the +// reader's preferences into a resolved locale. Everything the inline script in // index.html duplicates by hand is declared here. -/** - * Every catalog that ships, in the order the picker offers them. - * - * The last is not a language. It is the readable right-to-left pseudo-locale - * that exists so direction and truncation have something to prove themselves - * against, because the other three are all left to right. - */ +/** Every catalog that ships. The last is a pseudo-locale, not a language. */ export const CATALOG_IDS = ["en", "es", "fr", "ar-XB"] as const; /** The literal union of the ids above, formed with no assertion anywhere. */ export type CatalogId = (typeof CATALOG_IDS)[number]; /** - * The catalogs a preference list is allowed to select. - * - * The pseudo-locale is excluded because its primary subtag is ar, and an - * unfiltered walk would hand a reader who genuinely prefers Arabic a catalog of - * bracketed English. It stays reachable the only way it should be, by being - * chosen. + * The catalogs a preference list may select. The pseudo-locale's primary + * subtag is ar, and negotiation matches on that, so an unfiltered walk would + * serve bracketed English to a reader who wants Arabic. Excluding it here + * leaves it reachable the only way it should be, by being chosen. */ export const NEGOTIABLE_CATALOG_IDS = ["en", "es", "fr"] as const; -/** - * What the reader picked. The word rather than a catalog id means follow the - * machine, which is the default and is a choice like any other rather than the - * absence of one. - */ +/** The word rather than an id means follow the machine, the default. */ export type LocaleChoice = CatalogId | "system"; /** The storage key. The inline script in index.html spells this out by hand. */ export const LOCALE_STORAGE_KEY = "yart-locale"; -/** - * A resolved locale is three fields rather than one: which catalog supplies the - * strings, which language tag the platform formatters get, and which direction - * the document carries. - * - * The split is load-bearing rather than tidy. The pseudo-locale's id is a well - * formed language tag, so an engine carrying Arabic data would collate and - * format for Arabic if the id were passed through, and jsdom, Chromium and Node - * would each answer differently for reasons that have nothing to do with this - * code. Its strings really are English, so its tag is English and only its - * direction is borrowed. - */ +/** Three fields, because the pseudo-locale borrows a direction, not a tag. */ export interface ResolvedLocale { /** Which catalog supplies the strings. */ readonly catalog: CatalogId; @@ -56,18 +32,7 @@ export interface ResolvedLocale { readonly dir: "ltr" | "rtl"; } -/** - * The whole mapping, as literals. - * - * Direction is written out rather than asked of Intl.Locale.prototype - * getTextInfo, which is Chrome 130, Firefox 153 and Safari 17 against this - * app's floor of Chrome 111, Firefox 111 and Safari 16.4. Node 24 carries it, - * so a test of it under this runner would pass while Firefox 111 through 152 - * threw at the reader. - * - * Indexed only by a value of the closed union, so the lookup is total and there - * is no fallback arm here that nothing can reach. - */ +/** Direction written out: Intl.Locale getTextInfo is above the browser floor. */ const RESOLVED_LOCALES = { en: { catalog: "en", tag: "en-US", dir: "ltr" }, es: { catalog: "es", tag: "es-ES", dir: "ltr" }, @@ -75,15 +40,9 @@ const RESOLVED_LOCALES = { "ar-XB": { catalog: "ar-XB", tag: "en-US", dir: "rtl" }, } as const satisfies Readonly>; -/** - * Whether a value names a catalog that ships. The one gate between a - * reader-controlled string, from storage or from another document, and a lookup - * in a record keyed by the closed union. - */ +/** The one gate between a reader-controlled string and the closed union. */ export function isCatalogId(value: unknown): value is CatalogId { - // Widened for the search alone. The array is a closed tuple of catalog ids, - // so its own includes rejects an unknown argument outright, and the whole - // point here is to ask about one. + // Widened for the search: the tuple's own includes rejects an unknown. return (CATALOG_IDS as readonly unknown[]).includes(value); } @@ -93,10 +52,8 @@ export function isLocaleChoice(value: unknown): value is LocaleChoice { } /** - * A language tag's primary subtag, lowercased, which is the unit the lookup - * below matches on. Written with a search rather than a split so a tag carrying - * no separator takes a real branch instead of an index access that can never be - * absent. + * A language tag's primary subtag. A search rather than a split, so a tag with + * no separator takes a real branch rather than an index that is never absent. */ function primarySubtag(tag: string): string { const separator = tag.indexOf("-"); @@ -105,24 +62,9 @@ function primarySubtag(tag: string): string { } /** - * The rule that turns a choice plus the reader's preference list into the - * locale everything downstream reads. - * - * An explicit choice wins outright. Otherwise the preferences are walked in - * order and the first one whose primary subtag names a negotiable catalog is - * taken, which is the lookup rule from the language-tag matching standard - * reduced to the part this app can answer. Nothing matching is the base catalog - * rather than a failure, so the function is total and no caller has a throw to - * handle. - * - * Every answer is one of four module constants, so the same input returns the - * same object identity. The store below depends on that: it is what lets a - * snapshot reader be handed straight to React without a cache in front of it. - * - * This same rule is written a second time, as a literal, inside the blocking - * inline script in index.html. It has to be: that script runs before any module - * loads, so it cannot import this function. The guard in src/toolchain.test.ts - * is what holds the two copies together. + * Turns a choice plus a preference list into a resolved locale, always one of + * four module constants, so identity is stable. Written a second time as a + * literal in index.html's inline script; the parity guard holds the two. */ export function resolveLocale( choice: LocaleChoice, diff --git a/src/styles/_visually-hidden.scss b/src/styles/_visually-hidden.scss index 22c43a5..7013e1a 100644 --- a/src/styles/_visually-hidden.scss +++ b/src/styles/_visually-hidden.scss @@ -8,6 +8,12 @@ width: 1px; height: 1px; padding: 0; + + // The one px length on a spacing property in the tree, and the reason + // .stylelintrc.json re-declares declaration-property-unit-allowed-list for + // this file with margin left out. A unit allowed-list cannot say "a pixel or + // two", which is what the negative margin is: the classic off-screen clip, + // not spacing. Padding, gap and font-size here are still held. margin: -1px; overflow: hidden; clip-path: inset(50%); diff --git a/src/theme/resolveTheme.ts b/src/theme/resolveTheme.ts index 1d70b2c..6208733 100644 --- a/src/theme/resolveTheme.ts +++ b/src/theme/resolveTheme.ts @@ -2,14 +2,7 @@ // selector can match. Everything the inline script in index.html duplicates by // hand is declared here. -/** - * Every state the theme control offers, in the order it offers them. - * - * A value rather than a bare union, so the accepted set has one definition that - * both the hook's stored-choice check and the parity guard in - * src/toolchain.test.ts can read. The locale's own vocabulary derives its union - * from a tuple the same way, for the same reason. - */ +/** A value rather than a bare union, so the accepted set has one definition. */ export const THEME_CHOICES = ["light", "dark", "system"] as const; /** The literal union of the words above, formed with no assertion anywhere. */ @@ -24,15 +17,9 @@ export const THEME_STORAGE_KEY = "yart-theme"; export const PREFERS_DARK_QUERY = "(prefers-color-scheme: dark)"; /** - * The rule that turns a stored choice plus the operating system's preference into - * the concrete theme the document element carries. The attribute is never the - * word "system": that is a choice, not a theme, and a selector cannot resolve it. - * - * This same rule is written a second time, as a literal, inside the blocking - * inline script in index.html. It has to be: that script runs before any module - * loads, so it cannot import this function. A change to either one needs the - * same change to the other, and the parity guard in src/toolchain.test.ts is - * what fails when they stop agreeing. + * Turns a choice plus the system preference into the theme the document element + * carries; the attribute is never "system". Written a second time as a literal + * in index.html's inline script, and the parity guard holds the two together. */ export function resolveTheme( choice: ThemeChoice, diff --git a/src/theme/tokens.test.ts b/src/theme/tokens.test.ts index 3d40a8f..73e7cd1 100644 --- a/src/theme/tokens.test.ts +++ b/src/theme/tokens.test.ts @@ -1,5 +1,12 @@ // @vitest-environment node // +// Token layering and contrast, the theme script's placement in index.html, and +// the halves of the stylesheet rules stylelint has no way to express: an SCSS +// variable declared in a component sheet, a reference to a retired token, the +// global sheet's bounded px count, and the positive claim that the focus ring is +// drawn. The negative rules moved to .stylelintrc.json, where a violation is +// named at the line rather than at the end of a walk. +// // The stylesheet is the single source of truth for every colour in the app, so // this guard reads the shipped file rather than a copy of its values. Node // rather than the DOM environment for two measured reasons: the runner replaces @@ -79,59 +86,23 @@ const RETIRED_TOKENS = [ "--gray-900", ]; -// The four hex lengths CSS accepts, and nothing longer, so an identifier that -// merely starts with hex digits is not mistaken for a colour. -const HEX_COLOR = /#(?:[0-9a-f]{3,4}|[0-9a-f]{6}|[0-9a-f]{8})(?![0-9a-z-])/gi; - -// The other two forms CSS accepts for a fixed colour. A functional notation and -// a named colour are as fixed as a hex is, and neither flips with the theme, so -// a guard that reads hex alone waves both through. The boundaries exclude a -// hyphen, so var(--gray-50) and white-space are read as the identifiers they -// are rather than as colours. -const COLOR_FUNCTION = - /(? Number(magnitude) > PX_HAIRLINE_MAXIMUM) - .map(([length]) => length), - ...[...readable.matchAll(NON_REM_LENGTH)].map(([length]) => length), - ]; -} - -/** - * Every colour literal in a stylesheet, in each of the three forms CSS accepts - * for one. All of them rather than the first, so a file that reintroduces five - * reports five and is fixed once instead of five times. - */ -function colourLiterals(source: string): string[] { - return [HEX_COLOR, COLOR_FUNCTION, NAMED_COLOR].flatMap((matcher) => - [...source.matchAll(matcher)].map(([literal]) => literal), - ); -} - -/** - * The outline declarations in a stylesheet whose value cancels the focus ring, - * returned whole so the failure message names the declaration that has to go. - */ -function focusRingSuppressions(source: string): string[] { - return [...source.matchAll(OUTLINE_DECLARATION)] - .filter(([, value]) => - RING_CANCELLING_VALUE.test( - required(value, "the outline declaration's value"), - ), - ) - .map(([declaration]) => declaration.trim()); -} - /** * Every stylesheet under src/, found by walking rather than by a list, so a * stylesheet added by a later component is covered the day it lands instead of * the day someone remembers to add it here. Every extension rather than the - * module ones alone, because a shared partial and a global file are the two - * places a rule would otherwise be free to break. + * module ones alone, because a shared partial is a place a rule would otherwise + * be free to break. */ function findStylesheets(directory: string): string[] { const found: string[] = []; @@ -220,12 +138,11 @@ function findStylesheets(directory: string): string[] { return found; } -const stylesheets = findStylesheets(join(projectRoot, "src")); - -// The global file declares the hex primitives every other file reaches for -// through a token, so it is the one exemption from the colour half of the guard -// and from nothing else. -const componentStylesheets = stylesheets.filter((file) => file !== cssPath); +// The global file is read on its own terms below, by the two guards written +// against it, so it is held out of the walk rather than exempted inside one. +const componentStylesheets = findStylesheets(join(projectRoot, "src")).filter( + (file) => file !== cssPath, +); /** Every declaration in the file, keyed by selector then by property. */ function readBlocks(): Map> { @@ -329,26 +246,21 @@ function contrastRatio(a: string, b: string): number { * hover fill, only inherited body text does, and the focus ring sits outside the * border box on the parent surface rather than on the fill it surrounds. * + * Four text pairs are absent: axe decides them by value in the real-engine + * sweep, and CONTRAST-OVERLAP.md records that measurement per pair. + * * The two logo rows are measured by choice. The non-text contrast criterion * exempts logos and logotypes outright, so if a future surface change turns * either one red the correct answer is to drop the exempt pair deliberately, * never to lower a threshold to keep it. */ const PAIRS: Array<[string, string, number]> = [ - ["--color-text", "--color-surface", TEXT_CONTRAST_MINIMUM], - ["--color-text", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-text", "--color-surface-hover", TEXT_CONTRAST_MINIMUM], - ["--color-text-muted", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-text-muted", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-accent", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-accent", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], ["--color-error", "--color-surface", TEXT_CONTRAST_MINIMUM], ["--color-error", "--color-surface-raised", TEXT_CONTRAST_MINIMUM], - // The selected segment of the theme control: the surface colour laid on the - // accent, which is the one pairing in the app that reads a background token as - // a foreground. Measured here rather than by hand, so a later accent change - // cannot quietly take the control's label below the text threshold. - ["--color-surface", "--color-accent", TEXT_CONTRAST_MINIMUM], ["--color-border-strong", "--color-surface", NON_TEXT_CONTRAST_MINIMUM], [ "--color-border-strong", @@ -592,10 +504,11 @@ describe("the theme script in index.html", () => { }); }); -// A string check rather than a parse: the CSS parser throws outright on the -// inline comments in the table's stylesheet, and a guard written against the one -// module file that happens to have none would look like it worked. -describe("colour in the component stylesheets", () => { +// The halves of the old colour guard stylelint has no rule for: declaring an +// SCSS variable, and naming a token that no longer exists. A string check rather +// than a parse, because the CSS parser throws outright on the inline comments in +// the table's stylesheet. +describe("stray declarations in the component stylesheets", () => { it("finds the stylesheets by walking rather than by a list", () => { expect( componentStylesheets.length, @@ -603,7 +516,7 @@ describe("colour in the component stylesheets", () => { ).toBeGreaterThan(0); }); - it("leaves no colour literal, SCSS variable or retired token in any of them", () => { + it("leaves no SCSS variable or retired token in any of them", () => { const offenders: string[] = []; for (const file of componentStylesheets) { @@ -612,10 +525,6 @@ describe("colour in the component stylesheets", () => { const source = stripComments(readFileSync(file, "utf8")); const name = relative(projectRoot, file); - for (const literal of colourLiterals(source)) { - offenders.push(`${name}: holds the colour literal ${literal}`); - } - for (const variable of source.matchAll(SCSS_VARIABLE)) { offenders.push(`${name}: declares ${variable[0].trim()}`); } @@ -642,28 +551,17 @@ describe("colour in the component stylesheets", () => { }); }); -// The counterpart to the colour guard, and the reason the walk above takes -// every stylesheet rather than the module ones: the rule this keeps is that -// spacing and type are authored in rem through a token, so the layout follows -// the reader's browser font-size setting. A stylesheet written after this file -// inherits the rule by being walked, without anyone restating it. -describe("length in the stylesheets", () => { - it("leaves no px spacing in any component stylesheet", () => { - const offenders: string[] = []; - - for (const file of componentStylesheets) { - for (const length of offScaleLengths(readFileSync(file, "utf8"))) { - offenders.push( - `${relative(projectRoot, file)}: holds ${length}, which is spacing and belongs to a token`, - ); - } - } - - expect(offenders).toEqual([]); - }); - +// The half of the length rule stylelint's unit allowed-list has no way to say. +// It counts rather than forbids, and it reads src/index.css, where the two +// corner radii are px on purpose: growing with the reader's type would only +// distort the shape. +describe("length in the global stylesheet", () => { it("allows the global stylesheet the corner radii and nothing beside them", () => { - const found = offScaleLengths(readFileSync(cssPath, "utf8")); + const found = [ + ...stripComments(readFileSync(cssPath, "utf8")).matchAll(PX_VALUE), + ] + .filter(([, magnitude]) => Number(magnitude) > HAIRLINE_PX) + .map(([length]) => length); expect( found, @@ -685,123 +583,4 @@ describe("the focus ring", () => { "the global focus rule does not draw its outline from the ring token", ).toContain("--color-focus-ring"); }); - - it("is suppressed by no stylesheet", () => { - const offenders: string[] = []; - - // The global stylesheet is walked alongside the rest: a suppression there - // would cancel the rule from the same file that declares it. - for (const file of stylesheets) { - const source = stripComments(readFileSync(file, "utf8")); - - for (const suppression of focusRingSuppressions(source)) { - offenders.push( - `${relative(projectRoot, file)}: cancels the focus ring with ${suppression}`, - ); - } - } - - expect(offenders).toEqual([]); - }); -}); - -// The guards above read a clean tree, which is the one condition under which a -// guard that matches nothing and a guard that works are indistinguishable. Each -// spelling below is one a real author reaches for and one an earlier revision of -// these matchers passed, so the reach is asserted rather than assumed. -describe("the reach of the guards", () => { - it("sees a colour literal in every form CSS accepts for one", () => { - for (const declaration of [ - "color: #abc;", - "color: red;", - "background: rgb(1 2 3);", - "border-color: hsl(0 0% 0%);", - "background: transparent;", - "color: color-mix(in oklab, #fff, #000);", - ]) { - expect( - colourLiterals(declaration), - `${declaration} is invisible to the colour guard`, - ).not.toEqual([]); - } - }); - - it("reads a token reference and a property name as neither", () => { - for (const declaration of [ - "color: var(--color-text);", - "background: var(--gray-50);", - "white-space: nowrap;", - "background-color: var(--color-surface);", - ]) { - expect( - colourLiterals(declaration), - `${declaration} is reported as a colour literal`, - ).toEqual([]); - } - }); - - it("sees a length authored off the rem scale, whatever unit carries it", () => { - for (const declaration of [ - "padding: 1.5em;", - "margin: 12pt;", - "width: 2in;", - "gap: 3mm;", - "inline-size: 40ch;", - "margin-top: -1.5em;", - "padding: 24px;", - ]) { - expect( - offScaleLengths(declaration), - `${declaration} is invisible to the length guard`, - ).not.toEqual([]); - } - }); - - it("reads the rem scale, a hairline and a viewport measure as none of that", () => { - for (const declaration of [ - "padding: 1.5rem;", - "gap: 0.25rem;", - "width: 50%;", - "min-height: 100vh;", - "border-bottom: 1px solid var(--color-border);", - "margin: 0;", - "--space-2em: 1rem;", - ]) { - expect( - offScaleLengths(declaration), - `${declaration} is reported as an off-scale length`, - ).toEqual([]); - } - }); - - it("sees a suppressed focus ring however it is spelled", () => { - for (const declaration of [ - "outline: none;", - "outline: none !important;", - "outline-style: none;", - "a { color: var(--color-text); outline: none }", - "outline: 0 solid transparent;", - "outline-width: 0;", - "outline-width: 0rem;", - "outline-color: transparent;", - ]) { - expect( - focusRingSuppressions(declaration), - `${declaration} is invisible to the focus-ring guard`, - ).not.toEqual([]); - } - }); - - it("reads a drawn ring and an offset as neither", () => { - for (const declaration of [ - "outline: 2px solid var(--color-focus-ring);", - "outline: 0.125rem solid var(--color-focus-ring);", - "outline-offset: 2px;", - ]) { - expect( - focusRingSuppressions(declaration), - `${declaration} is reported as a suppression`, - ).toEqual([]); - } - }); }); diff --git a/src/toolchain.test.ts b/src/toolchain.test.ts index 7f76492..bce39fd 100644 --- a/src/toolchain.test.ts +++ b/src/toolchain.test.ts @@ -26,7 +26,6 @@ const guardFile = here.filename; interface Manifest { scripts?: Record; - browserslist?: unknown; [key: string]: unknown; } @@ -371,227 +370,6 @@ function findSourceFiles(directory: string): string[] { return found; } -/** - * Every stylesheet under a directory, both dialects, so a rule asked of the - * styling can be asked of all of it rather than of the dialect that happened to - * be checked. The global sheet is plain CSS and every component sheet is SCSS. - */ -function findStyleSheets(directory: string): string[] { - const found: string[] = []; - - for (const entry of readdirSync(directory, { withFileTypes: true })) { - const path = join(directory, entry.name); - - if (entry.isDirectory()) { - if (SKIPPED_DIRECTORIES.has(entry.name)) continue; - found.push(...findStyleSheets(path)); - } else if (/\.(css|scss)$/.test(entry.name)) { - found.push(path); - } - } - - return found; -} - -/** One declaration as written, with the property and the value already split. */ -interface StyleDeclaration { - readonly property: string; - readonly value: string; -} - -/** A stylesheet reduced to the two constructs the guards below ask about. */ -interface StyleSheetParts { - readonly declarations: readonly StyleDeclaration[]; - readonly selectors: readonly string[]; -} - -/** - * A stylesheet split into its declarations and its selectors. - * - * Constructs rather than raw text, which is this file's standard and is - * load-bearing here twice over. Every sheet in this tree carries paragraphs - * explaining itself, and the rules below are exactly the sort a comment states - * in order to say why it is being obeyed: a text search would go red on the - * explanation as readily as on a violation, at which point the guard gets - * deleted rather than kept. Quoted runs are carried through rather than - * dropped, because a selector matching an attribute value is a quoted run and - * one guard below reads it. - * - * At-rules are not declarations: an include, a use and a media prelude all end - * in a semicolon or open a block, and none of them sets a property. - */ -function styleSheetParts(source: string): StyleSheetParts { - const declarations: StyleDeclaration[] = []; - const selectors: string[] = []; - let buffer = ""; - let index = 0; - - const flushDeclaration = (): void => { - const text = buffer.trim(); - buffer = ""; - - if (text === "" || text.startsWith("@")) return; - - const colon = text.indexOf(":"); - - if (colon === -1) return; - - declarations.push({ - property: text.slice(0, colon).trim().toLowerCase(), - value: text - .slice(colon + 1) - .trim() - .toLowerCase(), - }); - }; - - while (index < source.length) { - const character = source[index]; - const following = source[index + 1]; - - if (character === '"' || character === "'") { - const close = source.indexOf(character, index + 1); - const end = close === -1 ? source.length : close + 1; - - buffer += source.slice(index, end); - index = end; - continue; - } - - if (character === "/" && following === "/") { - const end = source.indexOf("\n", index); - - index = end === -1 ? source.length : end; - continue; - } - - if (character === "/" && following === "*") { - const end = source.indexOf("*/", index + 2); - - index = end === -1 ? source.length : end + 2; - continue; - } - - if (character === "{") { - selectors.push(buffer.trim()); - buffer = ""; - index += 1; - continue; - } - - if (character === "}" || character === ";") { - flushDeclaration(); - index += 1; - continue; - } - - buffer += character; - index += 1; - } - - return { declarations, selectors }; -} - -/** - * The physical properties that name one end of the inline axis, so a sheet - * declaring one serves a left-to-right document and silently mis-serves a - * right-to-left one. - * - * The block axis is deliberately absent. Top and bottom mean the same thing - * whichever way the text runs, so banning them would be churn rather than a - * rule. - */ -const PHYSICAL_INLINE_PROPERTIES: ReadonlySet = new Set([ - "left", - "right", - "margin-left", - "margin-right", - "padding-left", - "padding-right", - "border-left", - "border-right", - "border-left-color", - "border-left-style", - "border-left-width", - "border-right-color", - "border-right-style", - "border-right-width", -]); - -/** - * The properties whose value, rather than whose name, can name one end of the - * inline axis. Each has a logical pair, start and end, that follows the - * document instead. - */ -const PHYSICAL_INLINE_VALUED_PROPERTIES: ReadonlySet = new Set([ - "text-align", - "float", - "clear", -]); - -/** The component holding the four glyphs that mean a direction. */ -const DIRECTIONAL_GLYPH_COMPONENT = "src/components/DataTable/Pagination.tsx"; - -/** Whether a node is JSX, in any of the three shapes the grammar allows. */ -function isJsx(node: ts.Node): boolean { - return ( - ts.isJsxElement(node) || - ts.isJsxSelfClosingElement(node) || - ts.isJsxFragment(node) - ); -} - -/** - * Whether a statement's own return is JSX, ignoring any nested function. - * - * A callback declared inside the branch returns whatever it returns, which is - * not the branch returning it, so the walk stops at a function boundary. - */ -function returnsJsx(node: ts.Node): boolean { - if (ts.isFunctionLike(node)) return false; - - if (ts.isReturnStatement(node)) { - return node.expression !== undefined && isJsx(node.expression); - } - - return ts.forEachChild(node, returnsJsx) ?? false; -} - -/** - * Every conditional in a file that picks between two pieces of JSX: a ternary - * with an element either side, or an if whose two branches each return one. - * - * A guarded render, which is the shape the table already uses, has JSX on one - * side and nothing on the other and is not one of these. - */ -function jsxAlternatives(file: ts.SourceFile): string[] { - const found: string[] = []; - - const visit = (node: ts.Node): void => { - if ( - ts.isConditionalExpression(node) && - isJsx(node.whenTrue) && - isJsx(node.whenFalse) - ) { - found.push(node.condition.getText(file)); - } - - if ( - ts.isIfStatement(node) && - node.elseStatement !== undefined && - returnsJsx(node.thenStatement) && - returnsJsx(node.elseStatement) - ) { - found.push(node.expression.getText(file)); - } - - node.forEachChild(visit); - }; - - file.forEachChild(visit); - return found; -} - const scannedFiles = findTestFiles(projectRoot).filter( (file) => file !== guardFile, ); @@ -605,45 +383,6 @@ function isEndToEndSpec(file: string): boolean { return relative(projectRoot, file).split(sep)[0] === E2E_DIRECTORY; } -/** - * Array.isArray narrows an unknown to any[], which reintroduces the untyped - * value the check was meant to remove. This narrows to unknown[] instead, so - * the elements stay unknown and have to be checked before they are used. - */ -function isUnknownArray(value: unknown): value is unknown[] { - return Array.isArray(value); -} - -/** browserslist wherever it is configured: inline, keyed by env, or in its own file. */ -function browserslistQueries(): unknown[] { - const configured = manifest.browserslist; - - if (isUnknownArray(configured)) return configured; - - if (configured && typeof configured === "object") { - return Object.values(configured as Record).flatMap( - (value) => (isUnknownArray(value) ? value : []), - ); - } - - const rcPath = join(projectRoot, ".browserslistrc"); - if (existsSync(rcPath)) { - return readFileSync(rcPath, "utf8") - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); - } - - return []; -} - -/** - * Queries whose meaning is decided by upstream data rather than by this manifest. - * Any of them lets a browserslist data release move the build output with no commit. - */ -const MOVING_QUERY = - /\b(defaults|last\s+\d+|dead|since\s+\d{4}|unreleased|maintained|current\s+node|node\s+current|extends|supports)\b|%/i; - const FAKES_CLOCK = /\buseFakeTimers\s*\(/; const CONFIGURES_CLOCK = /\bfakeTimers\s*:/; const IMPORTS_USER_EVENT = /from\s+["']@testing-library\/user-event["']/; @@ -685,13 +424,6 @@ const COVERAGE_EXCLUDE_PATTERNS = [ "src/**/*.d.ts", ]; -// The complete coverage include list. One entry, named here for the same -// reason its exclude sibling is written out: narrowing this to a subdirectory -// satisfies a hundred percent by shrinking the gate's input rather than by -// covering the code, and it is the sibling property the exclude guard does not -// reach. -const COVERAGE_INCLUDE_PATTERNS = ["src/**/*.{ts,tsx}"]; - // A suppression comment in any provider's spelling, matched against raw source // because a hint is itself a comment and blanking comments first would make the // guard vacuous. None exists in this tree: the standing convention is that an @@ -1216,69 +948,6 @@ function localeCallSites(file: ts.SourceFile): string[] { return found; } -/** The layer that renders any collection, and so may name none of them. */ -const SHARED_COMPONENT_DIRECTORY = "src/components"; - -/** - * The attributes whose string value a reader perceives. - * - * Two attributes carrying string values in that directory are deliberately not - * here. The sort state attribute takes one of three values the standard itself - * defines, and the live region's politeness setting takes one of two. Both are - * specification tokens rather than copy: assistive technology matches on them, - * so translating either would not localize anything, it would break the feature. - * They are English because the specification is, which is a different fact from - * a component holding a word for a reader. - */ -const READER_FACING_ATTRIBUTES = new Set([ - "aria-label", - "title", - "placeholder", - "alt", -]); - -/** - * Every string a reader could read out of a component: text rendered between - * tags, and a literal on one of the attributes above. - * - * Parsed rather than searched, which is this file's standard and is what makes - * the guard survivable. Every component in that directory carries paragraphs of - * prose explaining itself, and a search would fail on the explanation of the - * rule as readily as on a violation of it, at which point the guard gets - * deleted rather than obeyed. An attribute whose value is an expression is not - * a literal and is not collected: reading a string out of a prop is the shape - * this rule exists to require. - */ -function readerFacingLiterals(file: ts.SourceFile): string[] { - const found: string[] = []; - - const visit = (node: ts.Node): void => { - // Whitespace between elements is text too, so the letter is what separates - // a rendered word from the indentation around it. - if (ts.isJsxText(node) && /\p{L}/u.test(node.text)) { - found.push(node.text.trim()); - } - - if (ts.isJsxAttribute(node)) { - const name = node.name.getText(file); - const value = node.initializer; - - if ( - READER_FACING_ATTRIBUTES.has(name) && - value !== undefined && - ts.isStringLiteralLike(value) - ) { - found.push(`${name}="${value.text}"`); - } - } - - node.forEachChild(visit); - }; - - file.forEachChild(visit); - return found; -} - describe("toolchain baseline", () => { // This guard used to ban a list of names belonging to the runner that was // removed, and nothing else. A second runner arriving with a config of its own @@ -1342,61 +1011,6 @@ describe("toolchain baseline", () => { expect(script, "the test script names no project").toMatch(/--project[= ]/); }); - // The guard above covers the script a developer types and neither of the two - // the pipeline runs. Both are asserted as the properties that make them gates - // rather than as one string, for the same reason: adding a flag is free and - // dropping the one that matters is not. - // - // Without --coverage nothing measures coverage, so the threshold is never - // evaluated and coverage/lcov.info is never written, which the Sonar import - // reads as a silent zero rather than as an error. Without the browser project - // named, the browser script fans out to every project and reports the - // deterministic suite a second time as if it were the real-engine one. - // - // The end-to-end script is read the other way round: it must carry no - // coverage flag at all. The answer written beside its config is that this - // runner measures nothing and the hundred percent threshold stays over the - // deterministic project alone, and without this line that answer is a claim - // about intent that nothing checks. The failure it prevents is silent rather - // than loud: a coverage flag added later emits a second report over the same - // directory the static analysis import reads. - // - // Nothing else has to move for that carve, and both reasons are worth stating - // because both stop holding if the end-to-end specs are ever moved under the - // source directory. The coverage block of the build config is untouched, which - // is what keeps the four-pattern exclude set guard green, and the static - // analysis source set is the source directory, which is what keeps the derived - // inclusions guard green. Both hold because the specs live outside it. - it("keeps each pipeline test script carrying the flags its gate needs, and none it must not", () => { - const coverage = manifest.scripts?.["test:coverage"] ?? ""; - - expect(coverage, "the coverage script collects no coverage").toMatch( - /(^|\s)--coverage\b/, - ); - expect(coverage, "the coverage script names no project").toMatch( - /--project[= ]jsdom\b/, - ); - - expect( - manifest.scripts?.["test:browser"] ?? "", - "the browser script does not name the browser project", - ).toMatch(/--project[= ]browser\b/); - - // Read off the script itself rather than off an empty-string fallback. The - // two assertions above get existence for free because they match a flag - // positively and an absent script matches nothing; this one is the inverted - // case, where an absent script satisfies the pattern it is checked against. - // Without the line below, deleting the end-to-end script entirely passes - // the guard that exists to keep it honest. - const endToEnd = manifest.scripts?.["test:e2e"]; - - expect(endToEnd, "the end-to-end script is gone").toBeDefined(); - expect( - endToEnd as string, - "the end-to-end script collects coverage, which writes a second report into the directory the static analysis import reads", - ).not.toMatch(/(^|\s)--coverage\b/); - }); - // A report the upload step cannot collect is skipped, so the pipeline stays // green over an upload carrying nothing. it("keeps every configured report on a path the upload step collects", () => { @@ -1524,15 +1138,6 @@ describe("toolchain baseline", () => { } }); - // Most of the hook rule family is registered at warn rather than error by the - // plugin's own config, exhaustive-deps among them. Without the flag the gate - // exits zero with all of them reported, so neither the pipeline nor the - // pre-commit hook can fail on the rule that guards every dependency array in - // the tree. - it("fails the lint gate on a warning as well as an error", () => { - expect(manifest.scripts?.lint).toContain("--max-warnings 0"); - }); - // Nothing under src/ imports the icon or the manifest. index.html names each // by href and the bundler copies both out of public/ verbatim, so a rename // breaks neither the build nor the type check: it surfaces as a request for a @@ -1690,49 +1295,6 @@ describe("toolchain baseline", () => { }); }); - // browserslist is pinned to explicit versions like the rest of the manifest. A - // shared query such as "defaults" or "last 2 versions" would let upstream data - // releases move the build output without a commit. - it("pins browserslist to explicit versions rather than a moving query", () => { - const queries = browserslistQueries(); - - expect( - queries.length, - "browserslist is configured nowhere", - ).toBeGreaterThan(0); - - for (const query of queries) { - expect(typeof query, `${String(query)} is not a string`).toBe("string"); - expect(query as string, `${String(query)} is a moving query`).not.toMatch( - MOVING_QUERY, - ); - expect( - query as string, - `${String(query)} names no explicit version`, - ).toMatch(/\d/); - } - }); - - // CI runs the format check and so does the hook, which catches drift before it - // becomes a commit rather than after it becomes a push. - it("runs lint and the format check from the pre-commit hook", () => { - const hookPath = join(projectRoot, ".husky", "pre-commit"); - - expect(existsSync(hookPath), ".husky/pre-commit is missing").toBe(true); - - // Judged on live lines only: commenting the commands out and falling through to - // a bare exit disables the hook while leaving every expected string in the file. - const live = readFileSync(hookPath, "utf8") - .split("\n") - .map((line) => line.trim()) - .filter((line) => line && !line.startsWith("#")); - - expect(live.some((line) => line.includes("npm run lint"))).toBe(true); - expect(live.some((line) => line.includes("npm run format:check"))).toBe( - true, - ); - }); - // A faked clock plus the user input library deadlocks unless the library is told // which clock to advance, and a file that never restores the real clock leaks the // fake one into whatever runs next. Both were found the hard way during the @@ -1902,28 +1464,6 @@ describe("toolchain baseline", () => { expect(patterns?.toSorted()).toEqual(COVERAGE_EXCLUDE_PATTERNS.toSorted()); }); - // The threshold is the single line that turns the number into a gate, and the - // include list decides what the number is measured over. Both sit beside the - // exclude list and neither was guarded, so the gate could be reverted to a - // report, or fitted to a third of the tree, with every other guard green. - // Compared the same way the exclude list is: reordering is not a weakening - // and must not flap, while narrowing, widening or emptying must all fail. - it("keeps the coverage gate at a hundred percent over the whole source tree", () => { - expect( - coverageBlock(), - `${CONFIG_FILE} declares no hundred percent coverage threshold`, - ).toMatch(/thresholds\s*:\s*\{\s*100\s*:\s*true\s*,?\s*\}/); - - const patterns = coveragePatterns("include"); - - expect( - patterns, - `${CONFIG_FILE} declares no coverage include list`, - ).not.toBeNull(); - - expect(patterns?.toSorted()).toEqual(COVERAGE_INCLUDE_PATTERNS.toSorted()); - }); - // Sonar reads a file the coverage report excludes as main source and counts // every line of it as uncovered, which is how the same tree reported 92.9% // there and 98.5% here. The properties file states that the two lists have to @@ -2011,77 +1551,6 @@ describe("toolchain baseline", () => { expect(globalSheets("src/a11y.browser.test.tsx")).toEqual(shipped); }); - // Direction-dependent geometry is written once, on the inline axis, so one - // stylesheet serves both directions and there is no second artifact to keep in - // step. Five declarations in this tree were physical and were rewritten; a - // sixth arriving is invisible to every other check here, and is exactly the - // kind of thing that is correct in the browser the author happens to use. - // - // A test rather than a lint rule because the standard configuration in use - // carries no such rule, and the plugin that does is a new dependency for five - // declarations. This file already walks the tree and already parses what it - // asks about, so the guard costs a function rather than an install. - it("keeps direction-dependent geometry on the inline axis in every stylesheet", () => { - const sheets = findStyleSheets(join(projectRoot, "src")); - - expect( - sheets.length, - "src/ carries no stylesheet, so this guard is reading nothing", - ).toBeGreaterThan(0); - - const offenders = sheets.flatMap((sheet) => { - const name = relative(projectRoot, sheet); - - return styleSheetParts(readFileSync(sheet, "utf8")) - .declarations.filter( - ({ property, value }) => - PHYSICAL_INLINE_PROPERTIES.has(property) || - (PHYSICAL_INLINE_VALUED_PROPERTIES.has(property) && - /\b(?:left|right)\b/.test(value)), - ) - .map(({ property, value }) => `${name}: ${property}: ${value}`); - }); - - expect(offenders).toEqual([]); - }); - - // The two ways the rule above is most likely to be undone. Each is otherwise a - // sentence in a plan with nothing behind it. - // - // The mirror rule that turns the four page glyphs is written on the direction - // attribute for a measured reason: :dir() landed in Chrome 120 and this - // application's floor is 111, so the pseudo-class ships inert in the very - // browsers the floor exists to name. It is also the tidier-looking spelling, - // which is precisely why a later reader substitutes it. - it("selects direction on the attribute rather than on the pseudo-class", () => { - const offenders = findStyleSheets(join(projectRoot, "src")) - .filter((sheet) => sheet.endsWith(".scss")) - .flatMap((sheet) => - styleSheetParts(readFileSync(sheet, "utf8")) - .selectors.filter((selector) => selector.includes(":dir(")) - .map((selector) => `${relative(projectRoot, sheet)}: ${selector}`), - ); - - expect(offenders).toEqual([]); - }); - - // The other substitution: a branch in the component choosing between two glyph - // components on the direction, which is a prop and a coverage line for what one - // declaration does. A returning branch is invisible to the stylesheet guard - // above because it is not CSS, and invisible to the shared layer's literal - // guard because a glyph component is neither a text child nor a string, so it - // is asserted here or nowhere. - it("picks the page glyphs with a stylesheet rather than with a branch", () => { - const alternatives = jsxAlternatives( - moduleSource(DIRECTIONAL_GLYPH_COMPONENT), - ); - - expect( - alternatives, - `${DIRECTIONAL_GLYPH_COMPONENT} chooses between two elements on a condition`, - ).toEqual([]); - }); - // The dataset ceiling is written twice on purpose, once where a reader // evaluating this project reads and once where a reader of the code asks the // question. Two copies of one fact is how the provenance account came to have @@ -2280,85 +1749,89 @@ describe("toolchain baseline", () => { // The application resolves one locale and four surfaces follow it: the // catalog, the document element, the ordering of text and the grouping of // numbers. A fifth surface asking the platform for a locale of its own would - // reintroduce the defect this phase closed, and would do it invisibly, since - // a machine whose own preference is the base tag renders every one of them - // identically. Counting the call sites is the only thing that notices. + // reintroduce the defect the locale layer closed, and would do it invisibly, + // since a machine whose own preference is the base tag renders every one of + // them identically. // - // Test files are excluded, and deliberately. A test asserting a formatted - // string has to compute the expectation through the platform rather than type - // it, because the French group separator is a narrow no-break space and a - // typed literal fails on a difference no terminal renders. So the ban is on - // shipped call sites, not on the name. - it("asks the platform for a locale in exactly one module", () => { - const sources = findSourceFiles(join(projectRoot, "src")); - - expect( - sources.length, - "the source walk found no module under src/, so this guard read nothing", - ).toBeGreaterThan(0); - - const holders = new Map(); - - for (const path of sources) { - const name = relative(projectRoot, path).split(sep).join("/"); - const calls = localeCallSites(parse(readFileSync(path, "utf8"))); - - if (calls.length > 0) holders.set(name, calls); - } - - // The inline script resolves a locale of its own before any module loads, - // so it is walked here too. It reaches its answer through a literal map - // rather than through the platform, so it should contribute nothing, and if - // it ever grows a call this is where that shows up. - const stamped = localeCallSites(inlineScript()); - if (stamped.length > 0) holders.set("index.html", stamped); - + // The modules under src/ are held by the no-restricted-syntax rules in + // eslint.config.js. Two halves of that rule are outside what a lint rule can + // reach, and both are here. ESLint does not lint HTML, so the inline script is + // asserted here or nowhere; and a disallow rule cannot say that the formatter + // module still builds anything, so it passes just as happily on a formatter + // module with its caches deleted. + it("asks the platform for a locale only where the lint rule cannot reach", () => { + // The inline script resolves a locale of its own before any module loads. + // It reaches its answer through a literal map rather than through the + // platform, so it should contribute nothing. expect( - [...holders.keys()].toSorted(), - "something other than the formatter module asks the platform for a locale", - ).toEqual([FORMATTER_MODULE]); + localeCallSites(inlineScript()), + "the inline script in index.html asks the platform for a locale", + ).toEqual([]); - // Without this the assertion above passes just as happily on a formatter - // module that constructs nothing at all, which is the shape this guard - // would take the day someone deleted the caches it exists to protect. expect( - required( - holders.get(FORMATTER_MODULE), - `the call sites in ${FORMATTER_MODULE}`, + localeCallSites( + parse(readFileSync(join(projectRoot, FORMATTER_MODULE), "utf8")), ).toSorted(), "the formatter module no longer builds the three cached instances", ).toEqual(["Intl.Collator", "Intl.NumberFormat", "Intl.PluralRules"]); }); +}); - // The shared component layer renders any collection for any reader, and a - // literal there is a claim about which. The layer already may not import a - // domain type or the locale layer, and both of those are lint rules over - // imports; a hardcoded sentence needs no import and would pass them both. - // Every word that layer shows now arrives in a labels object, and this is - // what keeps the next one arriving the same way: a second locale added to a - // component still holding a literal is a second set of literals. - it("renders no reader-facing literal in the shared component layer", () => { - const components = findSourceFiles( - join(projectRoot, SHARED_COMPONENT_DIRECTORY), - ); +describe("the plugin rule sets the lint gate claims to run", () => { + // A flat-config block that spreads a shared config and then declares its own + // rules key replaces that config's rules wholesale rather than merging with + // them. The gate stays green, because the rules are simply absent. Same + // per-object replacement hazard eslint.config.js records for + // no-restricted-imports, one level up, and no disallow rule can state the + // positive claim that a rule set is still on. + // + // The one rule turned off on purpose: the new JSX transform needs no import + // in scope. Listed here so a second name joining it has to be deliberate. + const DELIBERATELY_OFF = ["react/react-in-jsx-scope"]; - expect( - components.length, - "the walk found no component, so this guard read nothing", - ).toBeGreaterThan(0); + const severityOf = (entry: unknown): unknown => + Array.isArray(entry) ? entry[0] : entry; - const holders = new Map(); + const isOff = (entry: unknown): boolean => { + const severity = severityOf(entry); + return severity === 0 || severity === "off" || severity === undefined; + }; - for (const path of components) { - const name = relative(projectRoot, path).split(sep).join("/"); - const literals = readerFacingLiterals(parse(readFileSync(path, "utf8"))); + it("has every rule of the React recommended set active", async () => { + const { ESLint } = await import("eslint"); + const react = (await import("eslint-plugin-react")).default; + + // calculateConfigForFile answers for a path with nothing behind it, so a + // rename would otherwise leave this guard green over a file that moved. + const target = join(projectRoot, "src/components/DataTable/TableHead.tsx"); + expect(existsSync(target), "the guard's sample file moved").toBe(true); + + const resolved: unknown = await new ESLint({ + cwd: projectRoot, + }).calculateConfigForFile(target); + const active = + (resolved as { rules?: Record }).rules ?? {}; + + const recommended = required( + react.configs.flat.recommended, + "the React recommended flat config", + ).rules; + + // The plugin ships a few of its own recommended entries at severity 0, so + // the claim is over the ones it actually enables. + const enabled = Object.entries(recommended ?? {}) + .filter(([, entry]) => !isOff(entry)) + .map(([name]) => name) + .filter((name) => !DELIBERATELY_OFF.includes(name)); - if (literals.length > 0) holders.set(name, literals); - } + expect( + enabled.length, + "the React recommended set is empty", + ).toBeGreaterThan(10); expect( - Object.fromEntries(holders), - "a component under the shared layer carries a string a reader can read", - ).toEqual({}); + enabled.filter((name) => isOff(active[name])), + "rules of the React recommended set are not on", + ).toEqual([]); }); });