Skip to content

types: refactor mark typings into shared modules#475

Merged
gka merged 3 commits into
mainfrom
codex/analyze-mark-component-check-issues
Feb 21, 2026
Merged

types: refactor mark typings into shared modules#475
gka merged 3 commits into
mainfrom
codex/analyze-mark-component-check-issues

Conversation

@gka

@gka gka commented Feb 21, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Extract axis/channel/data/mark/plot/scale types into src/lib/types/*, including the new axes.ts, so helpers can share a single source of truth
  • Update Plot, helpers, hooks, mark utilities, and transforms to import and use the centralized typings, addressing the common svelte-check issues across the mark components
  • Keep helper logic aligned with the new type exports while maintaining existing behavior

Testing

  • Not run (not requested)

@netlify

netlify Bot commented Feb 21, 2026

Copy link
Copy Markdown

Deploy Preview for svelteplot ready!

Name Link
🔨 Latest commit 2b1cb43
🔍 Latest deploy log https://app.netlify.com/projects/svelteplot/deploys/6999c7bd49377800086a6e2c
😎 Deploy Preview https://deploy-preview-475--svelteplot.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@gka gka changed the title Refactor mark typings into shared modules types: refactor mark typings into shared modules Feb 21, 2026
@gka gka requested a review from Copilot February 21, 2026 14:46
@github-actions

Copy link
Copy Markdown

📦 Preview package for this PR is published!

Version: 0.10.3-pr-475.0

Install it with:

npm install svelteplot@pr-475
# or install the specific version
npm install [email protected]

@github-actions

Copy link
Copy Markdown

📦 Preview package for this PR is published!

Version: 0.10.3-pr-475.1

Install it with:

npm install svelteplot@pr-475
# or install the specific version
npm install [email protected]

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.ts module with axis-specific types (AxisTickDatum, AxisXTick, AxisYTick, BaseAxisXOptions, BaseAxisYOptions)
  • Refactored PlotScaleFunction type 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 added Intl.DateTimeFormatOptions support
  • Changed data type constraints from T = ... to T extends object = ... for stricter type safety
  • Updated BaseAxisX and BaseAxisY components to use centralized types and make several props optional with defaults
  • Introduced ResolvedPlotOptions type to distinguish resolved options from raw user input
  • Added BivariantCallback wrapper for proper type variance in component props
  • Migrated some transforms (sort, rename) to use local TransformLike type instead of TransformArg

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

Comment thread src/lib/helpers/scales.ts
Comment on lines +39 to +45
out.range ||= () => [];
out.invert ||= (value: number) => value;
out.bandwidth ||= () => 0;
out.ticks ||= () => [];
out.quantiles ||= () => [];
out.thresholds ||= () => [];
out.domain ||= () => [];

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 = () => [];

Suggested change
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 = () => [];

Copilot uses AI. Check for mistakes.
Comment thread src/lib/transforms/sort.ts Outdated
Comment on lines +3 to +12
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
>;

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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>;

Copilot uses AI. Check for mistakes.
Comment on lines 3 to +9

type RenameChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName>>;
type ReplaceChannelsOptions = Partial<Record<ScaledChannelName, ScaledChannelName[]>>;
type TransformLike<T extends DataRecord = DataRecord> = { data: T[] } & Record<
string | symbol,
any
>;

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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>;

Copilot uses AI. Check for mistakes.
@@ -155,7 +155,7 @@ export function addEventHandlers<T extends DataRow>({

function invertScale(scale: PlotScale, position: number) {
if (scale.type === 'band') {

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
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.

Copilot uses AI. Check for mistakes.
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>;

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Comment thread src/lib/types/index.ts
Comment on lines +58 to +60
type BivariantCallback<TArgs extends unknown[], TResult> = {
bivarianceHack(...args: TArgs): TResult;
}['bivarianceHack'];

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
Record<ScaledChannelName | MarkStyleProps, ChannelAccessor> & { style: string }
>,
defaultColorProp: 'fill' | 'stroke' | null = null,
useScale: Record<ScaledChannelName, boolean>,

Copilot AI Feb 21, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
@github-actions

Copy link
Copy Markdown

📦 Preview package for this PR is published!

Version: 0.10.3-pr-475.2

Install it with:

npm install svelteplot@pr-475
# or install the specific version
npm install [email protected]

@gka gka merged commit 86c18ea into main Feb 21, 2026
9 checks passed
@gka gka deleted the codex/analyze-mark-component-check-issues branch February 21, 2026 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants