Apply suggestions from code review

Co-authored-by: Emily KL <4672118+emilykl@users.noreply.github.com>

Author: camdecoster

src/traces/image/attributes.ts

@@ -15,10 +15,6 @@ for (const name of cm) {
     zmaxDesc.push(`For the \`${name}\` colormodel, it is [${(cr.zmaxDflt || cr.max).join(', ')}].`);
 }
 
-/**
- * Image trace attributes.
- * Consumer-facing types are generated from the schema by generate_schema_types.mjs.
- */
 const attributes = {
     source: {
         valType: 'string',

src/types/ARCHITECTURE.md

@@ -38,7 +38,7 @@ re-exports.
 
 The split:
 
-- **Generated types** are the authoritative TypeScript representation of
+- **Generated types** (`src/types/generated`) are the authoritative TypeScript representation of
   the runtime schema. The schema itself is produced from Plotly's JS
   attribute files (`src/.../attributes.js`), which remain the source of
   truth: chain is **attribute files → `plot-schema.json` → generated
@@ -49,7 +49,7 @@ The split:
     - **Shared sub-interfaces** extracted from repeated subtrees (Font,
       FontArray, ColorBar, HoverLabel, Domain, Pattern, TickFormatStops,
       LegendGroupTitle).
-    - **Per-trace data interfaces** — all 49 (BarData, ScatterData,
+    - **Per-trace data interfaces** for all trace types (BarData, ScatterData,
       IndicatorData, etc.).
     - **Layout component interfaces** (LayoutAxis, Legend, Scene,
       Annotation, Shape, Slider, UpdateMenu, etc.) and the Layout
@@ -65,7 +65,8 @@ The split:
 
     Generated from `plot-schema.json` by `tasks/generate_schema_types.mjs`.
     Run `npm run schema` to regenerate.
-- **Hand-written types** cover everything the schema doesn't describe:
+- **Hand-written types** (everything in `src/types/` besides the `generated/` directory)
+  cover everything the schema doesn't describe:
   events, internal runtime state, public API function signatures,
   utility types (Color, Datum, MarkerSymbol, ErrorBar), behavioral types
   (ModeBarButton, Icon, etc.).

src/types/CONVERTING_ATTRIBUTES.md

@@ -135,25 +135,11 @@ for the canonical example. The full conversion changed:
 - `.default` added to `require('./attributes')` in `index.js` and `defaults.js`
 
 Note: Consumer-facing types for modebar (and all other layout components)
-are now generated from `plot-schema.json` by `tasks/generate_schema_types.mjs`,
+are generated from `plot-schema.json` by `tasks/generate_schema_types.mjs`,
 not from the individual `attributes.ts` files. The `attributes.ts` conversion
 still adds value by type-checking the source attribute definitions against
 `AttributeMap`.
 
-Schema output verified byte-identical (2547 bytes) before and after the
-conversion.
-
-## What stays hand-written
-
-The schema does not describe these — they remain in `src/types/`:
-
-- **Events** (`PlotMouseEvent`, `LegendClickEvent`, etc.)
-- **Public API function signatures** (`Plotly.newPlot`, `relayout`, ...)
-- **Internal types** (`FullLayout._modules`, `GraphDiv._fullData`, ...)
-- **Utility types** (`Color`, `Datum`, `TypedArray`, `MarkerSymbol`, etc.) —
-  these are the primitives the generator's emitted types reference.
-
-If you find yourself converting one of these, stop and ask.
 
 ## What if I need a type the schema doesn't describe well?
 
@@ -177,17 +163,13 @@ The schema-generated types are authoritative for everything in
 
 ## Schema-generated types
 
-All 49 trace data interfaces, layout component interfaces, and the Layout
+All trace data interfaces, layout component interfaces, and the Layout
 interface itself are generated from `plot-schema.json` by
 `tasks/generate_schema_types.mjs` (run via `npm run schema`). Individual
 trace and layout `attributes.js` files do **not** need to be converted
 for their types to appear in the public API — the schema generator
 covers them automatically.
 
-Converting an `attributes.js` file to TypeScript is still valuable because
-it type-checks the source definitions against `AttributeMap`, catching
-typos and structural errors at compile time.
-
 ## Order of conversion (for parallel work)
 
 Pick from this priority list. Lower-numbered items are smaller / simpler.

src/types/GENERATOR.md

@@ -7,7 +7,7 @@ types into `src/types/generated/schema.d.ts`:
 - Common enum aliases (Calendar, Dash, AxisType, PatternShape, XRef, YRef,
   TransitionEasing, PlotType)
 - Shared sub-interfaces (Font, ColorBar, HoverLabel, etc.)
-- 49 per-trace data interfaces (BarData, ScatterData, IndicatorData, ...)
+- Data interfaces for each trace type (BarData, ScatterData, IndicatorData, etc.)
 - Layout component interfaces (LayoutAxis, Legend, Scene, Annotation, etc.)
   and the Layout interface itself
 - Animation / frame / edits interfaces (AnimationOpts, Frame, Edits)
@@ -59,8 +59,8 @@ and leaf `valType`s produce the same fingerprint string.
 
 ### Phase 2: Shared interface extraction
 
-Containers that appear at least `MIN_OCCURRENCES` (= 5) times AND have at
-least `MIN_PROPERTIES` (= 4) properties become shared interfaces (Font,
+Containers that appear at least `MIN_OCCURRENCES` times AND have at
+least `MIN_PROPERTIES` properties become shared interfaces (Font,
 ColorBar, HoverLabel, etc.). PascalCase naming is controlled by
 `SHARED_NAME_OVERRIDES` so e.g. `colorbar` becomes `ColorBar` rather than
 `Colorbar`:
@@ -196,8 +196,6 @@ src/types/generated/schema.d.ts
 └── Animation / frames / config (AnimationOpts, Frame, Edits, ConfigBase)
 ```
 
-Regenerate with `npm run schema`.
-
 ## valType → TypeScript mapping
 
 Summary:
@@ -322,20 +320,13 @@ re-exported to consumers. Types inside the `_internal` namespace are still
 reachable via `_internal.X` (the namespace itself is exported by the
 wildcard) but their bare names are not.
 
-The wildcard is load-bearing: it removes the maintenance burden of keeping
-`lib/index.d.ts` in sync with new schema additions. If anyone ever swaps
-it for an explicit allowlist, restore the per-name re-export verifier
-that previously lived in `tasks/schema.mjs` (see git history) — otherwise
-new generated types will silently fail to surface in the public API.
 
 ## CI integration
 
 `npm run schema-typegen-diff-check` runs the generator and then verifies that
 both `test/plot-schema.json` and `src/types/generated/` are unchanged via
-`git diff --exit-code`. If either differs, exit code 1 — meaning either a
-developer changed the source schema but didn't commit the regenerated
-artifacts, or an attribute-file conversion silently altered the runtime
-schema and the change wasn't intentionally committed.
+`git diff --exit-code`. If either differs, the command fails with exit code 1
+and outputs the diff to the console.
 
 This is what makes the JS-to-TS conversion workflow safe: a correct
 conversion produces a byte-identical schema, so the check passes; an

src/types/README.md

@@ -14,7 +14,7 @@ This directory documents the TypeScript conversion in progress.
 - TypeScript build infrastructure: ✅ done
 - Public type surface in `src/types/`: ✅ done
 - `AttributeMap` validation machinery: ✅ done
-- **Schema-based type generator**: ✅ done — all 49 trace types + layout + shared interfaces
+- **Schema-based type generator**: ✅ done — all trace types + layout + shared interfaces
 - Consumer entry point (`lib/index.d.ts`, wired via `package.json#types`): ✅ done
 - CI gates (`typecheck` + `schema-typegen-diff-check`): ✅ done
 - First attribute file converted (modebar): ✅ done
@@ -31,7 +31,7 @@ The following are **auto-generated from `plot-schema.json`** by
 
 - Common enum aliases (Calendar, Dash, AxisType, PatternShape, XRef, YRef,
   TransitionEasing, PlotType)
-- All 49 per-trace data interfaces (BarData, ScatterData, IndicatorData, ...)
+- Data interfaces for each trace type (BarData, ScatterData, IndicatorData, etc.)
 - Layout component interfaces (LayoutAxis, Legend, Scene, Annotation,
   Shape, Slider, UpdateMenu, etc.) and the Layout interface itself
 - Shared sub-interfaces (Font, ColorBar, HoverLabel, LegendGroupTitle, etc.)

src/types/SETUP.md

@@ -4,19 +4,16 @@ Quick reference for the TypeScript toolchain in plotly.js.
 
 ## What's installed
 
-- **TypeScript** (`typescript ^5.9.3`) — type checker only, never emits JS
-- **ts-node** (`^10.9.2`) — runs TS scripts directly (build helpers)
-- **@types/node**, **@types/d3** — third-party type definitions
-- esbuild handles `.ts` natively for bundling — no plugins needed
+The following dev dependencies are used for maintaining plotly.js types:
 
-## Two tools, two jobs
+- `typescript` — used for type-checking only
+- `ts-node` — used for running TS scripts (build helpers)
+- `@types/node`, `@types/d3` — provide third-party type definitions
 
-| Tool | Job |
-|---|---|
-| **esbuild** | Bundle for production. Strips types, transpiles to ES2016, emits `dist/plotly.js`. Fast (~450ms full build). Does **not** check types. |
-| **tsc** | Type-check only. Reads `.ts` and `.js` (with `allowJs: true`), reports errors, no output. Slower (~2-5s) but catches bugs. |
+Note: esbuild handles `.ts` files natively for bundling, so no extra plugins are needed for the bundling process.
+
+`tsconfig.json` sets `noEmit: true` so that tsc never writes files. esbuild is the build system; tsc is the verifier.
 
-`tsconfig.json` sets `noEmit: true` so tsc never writes files. esbuild is the build system; tsc is the verifier.
 
 ## Configuration
 
@@ -31,10 +28,9 @@ Both target ES2016. `strict: true` is on in `tsconfig.json` — the type system
 npm run typecheck         # tsc --noEmit, errors reported, no output
 npm run typecheck-watch   # incremental rechecking on change
 
-npm run schema            # rebuild test/plot-schema.json + regenerate types
-npm run schema-typegen-diff-check      # regenerate + verify no uncommitted drift in test/plot-schema.json or schema.d.ts
-npm run bundle            # esbuild → dist/plotly.js
-npm run build             # full production build
+npm run schema            # rebuild test/plot-schema.json + regenerate types under src/types/generated/
+npm run schema-typegen-diff-check      # regenerate + verify no changes to test/plot-schema.json or src/types/generated/schema.d.ts
+npm run build             # full production build (regenerate all files under `dist/`)
 ```
 
 ## Workflows
@@ -77,12 +73,3 @@ var attributes = require('./attributes').default;
 
 This shows up when converting `attributes.js` → `attributes.ts`. See [CONVERTING_ATTRIBUTES.md](CONVERTING_ATTRIBUTES.md) step 4.
 
-## Performance
-
-For a codebase of ~750 source files:
-
-| Operation | Time |
-|---|---|
-| `tsc --noEmit` cold | ~2-5s |
-| `tsc --noEmit --watch` incremental | ~100ms |
-| esbuild full bundle | ~450ms |

tasks/generate_schema_types.mjs

@@ -28,8 +28,9 @@ let META_KEYS = new Set();
 // ---------------------------------------------------------------------------
 // Common enum types — discovered from the schema at generation time and
 // emitted as named type aliases in the output. Each anchor specifies a
-// PascalCase type name and a `match(key, path, values)` predicate. The first
-// enumerated attribute whose match returns true defines the alias's values.
+// PascalCase type name and a `match(key, path, values)` predicate. If multiple
+// enumerated attributes match the predicate, the one with the largest set of values
+//  is chosen.
 // ---------------------------------------------------------------------------
 
 const COMMON_TYPE_ANCHORS = [

tasks/schema.mjs

@@ -72,14 +72,8 @@ await build(localDevConfig);
 // output plot-schema JSON
 makeSchema(pathToPlotly, pathToSchema);
 
-// generate TypeScript types from the schema — traces, layout, common enum
-// aliases, animation/frame/config interfaces, and an `_internal` namespace.
-//
-// New schema-derived types automatically reach `plotly.js` consumers because
-// `lib/index.d.ts` uses `export type * from '../src/types/generated/schema'`.
-// If you ever swap that wildcard for an explicit allowlist, restore the
-// per-name re-export verifier that lived here (see git history) — otherwise
-// new types will silently fail to surface in the public API.
+// generate TypeScript types from the schema
+// and write to `src/types/generated/schema.d.ts`
 const schema = JSON.parse(fs.readFileSync(pathToSchema, 'utf-8'));
 const pathToGeneratedTypes = path.join(constants.pathToSrc, 'types/generated/schema.d.ts');
 generateSchemaTypes(schema, pathToGeneratedTypes);

tsconfig.json

@@ -26,7 +26,6 @@
     "outDir": "./dist",
     "noEmit": true,
 
-    // Type checking - start strict, loosen if necessary
     "strict": true,
 
     // Linting