types: refactor mark typings into shared modules#475
Conversation
✅ Deploy Preview for svelteplot ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-475
# or install the specific version
npm install [email protected] |
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-475
# or install the specific version
npm install [email protected] |
There was a problem hiding this comment.
Pull request overview
This PR refactors the type system by extracting mark-related types into centralized modules under src/lib/types/*, creating a single source of truth for types used across helpers, marks, transforms, and the core plotting system. The primary goal is to improve type safety and address common svelte-check issues across mark components.
Changes:
- Created new
src/lib/types/axes.tsmodule with axis-specific types (AxisTickDatum, AxisXTick, AxisYTick, BaseAxisXOptions, BaseAxisYOptions) - Refactored
PlotScaleFunctiontype to provide a broad interface for D3 scale functions with normalized method implementations - Updated tick format types to support multi-line labels (
string | string[]) and addedIntl.DateTimeFormatOptionssupport - Changed data type constraints from
T = ...toT extends object = ...for stricter type safety - Updated BaseAxisX and BaseAxisY components to use centralized types and make several props optional with defaults
- Introduced
ResolvedPlotOptionstype to distinguish resolved options from raw user input - Added
BivariantCallbackwrapper for proper type variance in component props - Migrated some transforms (sort, rename) to use local
TransformLiketype instead ofTransformArg
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/types/scale.ts | Updated TickFormatFunction return type to support multi-line labels, changed to PlotScaleFunction interface, added date format options |
| src/lib/types/plot.ts | Changed PlotOptions to ResolvedPlotOptions in PlotState, added resolved scale option types |
| src/lib/types/mark.ts | Added 'axisX', 'axisY', 'link' mark types, changed BaseMarkProps default generic, added title prop |
| src/lib/types/index.ts | Added BivariantCallback type, exported new axes module |
| src/lib/types/data.ts | Changed generic constraint from T = to T extends object = for stricter typing |
| src/lib/types/channel.ts | Changed Channels to Partial<Record<...>>, added BivariantCallback type |
| src/lib/types/axes.ts | New file defining axis-specific types for tick data and options |
| src/lib/transforms/sort.ts | Replaced TransformArg with local TransformLike type, updated return types |
| src/lib/transforms/rename.ts | Replaced TransformArg with local TransformLike type, updated return types |
| src/lib/marks/helpers/waffle.ts | Updated scale function cast to use as unknown as ScaleBand |
| src/lib/marks/helpers/events.ts | Updated scale function cast to use as unknown as ScaleBand |
| src/lib/marks/helpers/TrailCanvas.svelte | Added extends object constraint to generic type parameter |
| src/lib/marks/helpers/BaseAxisY.svelte | Updated to use centralized types, made props optional with defaults, improved type safety |
| src/lib/marks/helpers/BaseAxisX.svelte | Updated to use centralized types, made props optional with defaults, improved type safety |
| src/lib/hooks/usePlot.svelte.ts | Changed PlotOptions to ResolvedPlotOptions |
| src/lib/helpers/scales.ts | Added normalizeScaleFn to provide default method implementations for PlotScaleFunction |
| src/lib/helpers/resolve.ts | Updated channel types to include MarkStyleProps union, added casting for type compatibility |
| src/lib/helpers/autoTicks.ts | Made ticks and interval parameters optional |
| src/lib/core/Plot.svelte | Updated function return types to ResolvedPlotOptions, added type cast |
| out.range ||= () => []; | ||
| out.invert ||= (value: number) => value; | ||
| out.bandwidth ||= () => 0; | ||
| out.ticks ||= () => []; | ||
| out.quantiles ||= () => []; | ||
| out.thresholds ||= () => []; | ||
| out.domain ||= () => []; |
There was a problem hiding this comment.
The normalizeScaleFn function uses nullish coalescing assignment (||=) to add default method implementations. However, this will not add the methods if they already exist but are falsy (like 0, false, ''). Consider using proper existence checks with in operator or checking for undefined specifically to avoid this edge case. For example: if (!('range' in out)) out.range = () => [];
| out.range ||= () => []; | |
| out.invert ||= (value: number) => value; | |
| out.bandwidth ||= () => 0; | |
| out.ticks ||= () => []; | |
| out.quantiles ||= () => []; | |
| out.thresholds ||= () => []; | |
| out.domain ||= () => []; | |
| if (out.range === undefined) out.range = () => []; | |
| if (out.invert === undefined) out.invert = (value: number) => value; | |
| if (out.bandwidth === undefined) out.bandwidth = () => 0; | |
| if (out.ticks === undefined) out.ticks = () => []; | |
| if (out.quantiles === undefined) out.quantiles = () => []; | |
| if (out.thresholds === undefined) out.thresholds = () => []; | |
| if (out.domain === undefined) out.domain = () => []; |
| import type { DataRecord } from '../types/index.js'; | ||
| import { shuffler } from 'd3-array'; | ||
| import { randomLcg } from 'd3-random'; | ||
|
|
||
| export const SORT_KEY = Symbol('sortKey'); | ||
| export const IS_SORTED = Symbol('isSorted'); | ||
| type TransformLike<T extends DataRecord = DataRecord> = { data: T[] } & Record< | ||
| string | symbol, | ||
| any | ||
| >; |
There was a problem hiding this comment.
The TransformLike type definition { data: T[] } & Record<string | symbol, any> is less specific than the standard TransformArg<T> type, which properly combines Channels<T> and BaseMarkProps<T>. This looser typing could allow invalid properties to pass through without type checking. Consider using the existing TransformArg<T> type instead, or if you need more flexibility, consider something like { data: T[] } & Partial<Channels<T> & BaseMarkProps<T>> & Record<string | symbol, any> to maintain better type safety while allowing additional properties.
| import type { DataRecord } from '../types/index.js'; | |
| import { shuffler } from 'd3-array'; | |
| import { randomLcg } from 'd3-random'; | |
| export const SORT_KEY = Symbol('sortKey'); | |
| export const IS_SORTED = Symbol('isSorted'); | |
| type TransformLike<T extends DataRecord = DataRecord> = { data: T[] } & Record< | |
| string | symbol, | |
| any | |
| >; | |
| import type { DataRecord, TransformArg } from '../types/index.js'; | |
| import { shuffler } from 'd3-array'; | |
| import { randomLcg } from 'd3-random'; | |
| export const SORT_KEY = Symbol('sortKey'); | |
| export const IS_SORTED = Symbol('isSorted'); | |
| type TransformLike<T extends DataRecord = DataRecord> = TransformArg<T>; |
|
|
||
| type RenameChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName>>; | ||
| type ReplaceChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName[]>>; | ||
| type TransformLike<T extends DataRecord = DataRecord> = { data: T[] } & Record< | ||
| string | symbol, | ||
| any | ||
| >; |
There was a problem hiding this comment.
The TransformLike type definition { data: T[] } & Record<string | symbol, any> is less specific than the standard TransformArg<T> type. This looser typing could allow invalid properties to pass through without type checking. Consider using the existing TransformArg<T> type instead, or if you need more flexibility, extend it properly to maintain type safety.
| type RenameChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName>>; | |
| type ReplaceChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName[]>>; | |
| type TransformLike<T extends DataRecord = DataRecord> = { data: T[] } & Record< | |
| string | symbol, | |
| any | |
| >; | |
| import type { TransformArg } from '../types/index.js'; | |
| type RenameChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName>>; | |
| type ReplaceChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName[]>>; | |
| type TransformLike<T extends DataRecord = DataRecord> = TransformArg<T>; |
| @@ -155,7 +155,7 @@ export function addEventHandlers<T extends DataRow>({ | |||
|
|
|||
| function invertScale(scale: PlotScale, position: number) { | |||
| if (scale.type === 'band') { | |||
There was a problem hiding this comment.
Consider adding step() method to the PlotScaleFunction type definition to avoid the need for this as unknown as ScaleBand cast. The step() method is commonly used with band scales and would make the type more complete. Alternatively, document that this cast is necessary for accessing scale-type-specific methods not included in the broad PlotScaleFunction interface.
| if (scale.type === 'band') { | |
| if (scale.type === 'band') { | |
| // NOTE: PlotScale.fn is typed as a broad plot scale function that does not include | |
| // band-scale-specific methods like `step()`. For band scales only (guarded by | |
| // `scale.type === 'band'`), we narrow the type to `ScaleBand` so we can safely use | |
| // `range()`, `domain()`, and `step()`. This cast is intentionally used here for | |
| // accessing these scale-type-specific methods. |
| const xScale = scales[x as 'x' | 'y']; | ||
| const yScale = scales[y as 'x' | 'y']; | ||
| const xBandScale = xScale.fn as ScaleBand<string | number>; | ||
| const xBandScale = xScale.fn as unknown as ScaleBand<string | number>; |
There was a problem hiding this comment.
Consider adding step(), paddingInner(), paddingOuter(), and other commonly-used band scale methods to the PlotScaleFunction type definition to reduce the need for type casts like this one throughout the codebase.
| type BivariantCallback<TArgs extends unknown[], TResult> = { | ||
| bivarianceHack(...args: TArgs): TResult; | ||
| }['bivarianceHack']; |
There was a problem hiding this comment.
The BivariantCallback type is defined in both src/lib/types/index.ts and src/lib/types/channel.ts. This duplication should be avoided to maintain a single source of truth. Consider removing this definition from index.ts and importing it from channel.ts instead, or defining it once in a shared location and importing it in both places.
| Record<ScaledChannelName | MarkStyleProps, ChannelAccessor> & { style: string } | ||
| >, | ||
| defaultColorProp: 'fill' | 'stroke' | null = null, | ||
| useScale: Record<ScaledChannelName, boolean>, |
There was a problem hiding this comment.
The useScale parameter in resolveStyles has type Record<ScaledChannelName, boolean>, which requires all scaled channel names to be present. However, callers are passing partial objects like { y: true } with an as any cast to work around this. Consider changing the type to Partial<Record<ScaledChannelName, boolean>> to make the API more flexible and eliminate the need for these type casts.
|
📦 Preview package for this PR is published! Version: Install it with: npm install svelteplot@pr-475
# or install the specific version
npm install [email protected] |
Summary
src/lib/types/*, including the newaxes.ts, so helpers can share a single source of truthPlot, helpers, hooks, mark utilities, and transforms to import and use the centralized typings, addressing the commonsvelte-checkissues across the mark componentsTesting