From 04271f30f780920d4b0146bfefd29b008f990ca3 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 17:59:09 +0100 Subject: [PATCH 1/9] Export runtime type guards for zarr tree nodes (#97) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ZarrTree` admits a group or a `LazyZarrArray` at every key and shipped no way to tell them apart, so consumers hand-rolled the check — and the obvious `typeof node === 'object'` test is wrong, because a lazy array is an object too and its own properties (`get`) then read as child keys. That had been open-coded in four places. `zarrextra` now exports the discrimination itself, in `treeNodes.ts`: `isLazyZarrArray` / `isZarrGroup`, the `getChildNode` / `getChildGroup` / `getChildArray` accessors most call sites actually want, and `getNodeAttrs` / `getArrayMetadata` for the symbol-keyed payloads. `getArrayDtype` folds v2's numpy typestrings and v3's names into one vocabulary — zarrita's own `DataType`, not a bare string — so a check made against tree metadata and the same check made against an opened array cannot disagree. `isTextDataType` is that shared definition of "does this need decoding to strings", covering v3 `string`, v2 fixed-width unicode/bytes and `v2:object`; zarrita's `isDataType(dtype, 'string')` excludes the last, and testing for one spelling without the other is what makes a reader hand back raw integer codes where labels were expected. `LazyZarrArray`'s `ZARRAY_KEY` payload is typed as `ZarrArrayMetadata` instead of an untyped record, so `dtype` and `data_type` can no longer be read without narrowing. The union keeps an unrecognised-record member on purpose: this is unvalidated JSON, and zarr v3 permits extension dtypes written as objects, so a strict two-member union would either lie or have to fail the store open. In `@spatialdata/core`, `parsed` is narrowed to a group once in `AbstractElement` so no element subclass sees the union, and `classifyObsColumnNode`, `getObsGroup` and `loadElements` drop their casts. `AnnDataSource.hasTextValues` now asks `isTextDataType` about an opened array's dtype, so the kind a UI sees before loading a column and the decoding it gets on load come from one definition. `readNullableArray`, `isNullableEncoding` and `NULLABLE_ENCODING_KINDS` are public too: guards make "is this group a categorical or a nullable column?" expressible, but only the semantics make it answerable without every consumer re-deriving AnnData's on-disk layout. Co-Authored-By: Claude Opus 5 --- .changeset/zarr-tree-node-guards.md | 39 ++++ packages/core/src/index.ts | 10 + packages/core/src/models/VAnnDataSource.ts | 8 +- packages/core/src/models/index.ts | 87 ++++---- packages/core/src/types.ts | 32 ++- packages/zarrextra/src/index.ts | 27 ++- packages/zarrextra/src/treeNodes.ts | 223 +++++++++++++++++++++ packages/zarrextra/src/types.ts | 63 +++++- packages/zarrextra/tests/treeNodes.spec.ts | 205 +++++++++++++++++++ 9 files changed, 628 insertions(+), 66 deletions(-) create mode 100644 .changeset/zarr-tree-node-guards.md create mode 100644 packages/zarrextra/src/treeNodes.ts create mode 100644 packages/zarrextra/tests/treeNodes.spec.ts diff --git a/.changeset/zarr-tree-node-guards.md b/.changeset/zarr-tree-node-guards.md new file mode 100644 index 00000000..59c4c6c5 --- /dev/null +++ b/.changeset/zarr-tree-node-guards.md @@ -0,0 +1,39 @@ +--- +'zarrextra': minor +'@spatialdata/core': minor +--- + +Export runtime type guards and accessors for zarr tree nodes. + +`ZarrTree` admits a group or a `LazyZarrArray` at every key and shipped no way to tell +them apart, so consumers hand-rolled the check — and the obvious `typeof node === 'object'` +test is wrong, because a lazy array is an object too and its own properties (`get`) then +read as child keys. `zarrextra` now exports the discrimination itself: + +- `isLazyZarrArray` / `isZarrGroup` — the guards, discriminating on `ZARRAY_KEY`. +- `getChildNode` / `getChildGroup` / `getChildArray` — "the node at this path, if it is + the kind I need", which is the shape most call sites actually want. +- `getNodeAttrs` / `getArrayMetadata` — the symbol-keyed payloads of either kind of node. +- `getArrayDtype` / `normalizeDtype` — the data type of an array node, from consolidated + metadata alone, with v2's numpy typestrings (` { const arr = await zarrOpen(this.storeRoot.resolve(path), { kind: 'array' }); - return arr.is('string') || arr.is('object'); + return isTextDataType(arr.dtype); } /** diff --git a/packages/core/src/models/index.ts b/packages/core/src/models/index.ts index 76d099f3..340b0655 100644 --- a/packages/core/src/models/index.ts +++ b/packages/core/src/models/index.ts @@ -20,15 +20,15 @@ import { type BaseTransformation, Identity, parseTransforms } from '../transform import type { BadFileHandler, ElementName, - LazyZarrArray, Result, SDataProps, TableColumnData, TableColumnKind, ZAttrsAny, + ZarrDataType, ZarrTree, } from '../types'; -import { ATTRS_KEY, Err, Ok, ZARRAY_KEY } from '../types'; +import { Err, getArrayDtype, getChildGroup, getNodeAttrs, isTextDataType, Ok } from '../types'; import { NULLABLE_ENCODING_KINDS } from './nullableArrays'; import SpatialDataPointsSource from './VPointsSource'; import SpatialDataShapesSource from './VShapesSource'; @@ -60,7 +60,15 @@ abstract class AbstractElement { readonly url?: string; protected readonly sdata: SDataProps; protected readonly rawAttrs: ZAttrsAny; - protected readonly parsed: ZarrTree | LazyZarrArray; + /** + * The element's own node in the tree. + * + * Narrowed to a group here, once, so no subclass has to re-litigate the + * union: every SpatialData element is a group on disk (a multiscale, a + * dataframe, an AnnData), and one that arrives as an array is a store we + * cannot read, not a case to handle further down. + */ + protected readonly parsed: ZarrTree; constructor({ sdata, name, key }: ElementParams) { this.sdata = sdata; @@ -73,15 +81,16 @@ abstract class AbstractElement { if (!tree) { throw new Error('Tree store contents not available'); } - if (!(name in tree)) { + const elementTypeGroup = getChildGroup(tree, name); + if (!elementTypeGroup) { throw new Error(`Unknown element type: ${name}`); } - const p1 = tree[name] as ZarrTree; - if (!(key in p1)) { + const elementGroup = getChildGroup(elementTypeGroup, key); + if (!elementGroup) { throw new Error(`Unknown element key: ${key}`); } - this.parsed = p1[key]; - this.rawAttrs = ((p1[key] as ZarrTree)[ATTRS_KEY] as ZAttrsAny) ?? {}; + this.parsed = elementGroup; + this.rawAttrs = getNodeAttrs(elementGroup) ?? {}; } } @@ -229,22 +238,22 @@ const OBS_KIND_BY_ENCODING: Record = { }; /** - * Classify a dtype from either zarr generation. + * Classify a normalised zarr data type. * - * v3 spells them out (`float64`, `bool`, `string`); v2 uses numpy typestrings - * (`|=]/, '').charAt(0); - if (code === 'b') return 'boolean'; - if (code === 'O' || code === 'S' || code === 'U') return 'string'; - if (code === 'i' || code === 'u' || code === 'f') return 'numeric'; - return undefined; + return 'numeric'; } /** @@ -260,9 +269,7 @@ function classifyObsDtype(dtype: string): TableColumnKind | undefined { * consumer already handles as "decide some other way". */ export function classifyObsColumnNode(node: unknown): TableColumnKind | undefined { - if (!node || typeof node !== 'object') return undefined; - - const attrs = (node as ZarrTree)[ATTRS_KEY]; + const attrs = getNodeAttrs(node); const encoding = attrs?.['encoding-type']; if (typeof encoding === 'string' && OBS_KIND_BY_ENCODING[encoding]) { return OBS_KIND_BY_ENCODING[encoding]; @@ -271,11 +278,10 @@ export function classifyObsColumnNode(node: unknown): TableColumnKind | undefine // of an `encoding-type` — the same form `_loadColumn` has always had to accept. if (attrs && 'categories' in attrs) return 'categorical'; - const arrayMetadata = (node as LazyZarrArray)[ZARRAY_KEY] as ZAttrsAny | undefined; - if (!arrayMetadata) return undefined; - // `data_type` is zarr v3, `dtype` is v2. - const dtype = arrayMetadata.data_type ?? arrayMetadata.dtype; - return typeof dtype === 'string' ? classifyObsDtype(dtype) : undefined; + // A group that said nothing about itself above (a nullable column, a + // categorical) has no dtype of its own, and reports `undefined` here. + const dtype = getArrayDtype(node); + return dtype ? classifyObsDtype(dtype) : undefined; } // ============================================ @@ -344,23 +350,9 @@ export class TableElement extends AbstractElement<'tables'> { /** * The `obs` group node, or `undefined` when this table has no `obs` or the * node turns out to be an array rather than a group. - * - * `ZarrTree`'s index signature admits a `LazyZarrArray` at every key, and a - * lazy array is an object too — so a `typeof === 'object'` test alone lets one - * through, and its own properties would then read as obs column names. - * `ZARRAY_KEY` is the discriminator (it is required on `LazyZarrArray`, - * absent on groups), the same one `serializeZarrTree` uses. */ private getObsGroup(): ZarrTree | undefined { - const tableNode = this.parsed; - if (ZARRAY_KEY in tableNode) { - return undefined; - } - const obsNode = tableNode.obs; - if (!obsNode || typeof obsNode !== 'object' || ZARRAY_KEY in obsNode) { - return undefined; - } - return obsNode; + return getChildGroup(this.parsed, 'obs'); } /** @@ -370,7 +362,7 @@ export class TableElement extends AbstractElement<'tables'> { * name (e.g. `cell_id`). */ getObsIndexColumnName(): string | undefined { - const indexName = this.getObsGroup()?.[ATTRS_KEY]?._index; + const indexName = getNodeAttrs(this.getObsGroup())?._index; return typeof indexName === 'string' ? indexName : undefined; } @@ -782,11 +774,12 @@ export function loadElements( if (!tree) { throw new Error('Tree store contents not available'); } - if (!(name in tree)) { + const elementTypeGroup = getChildGroup(tree, name); + if (!elementTypeGroup) { return undefined; } - const keys = Object.keys(tree[name] as object); + const keys = Object.keys(elementTypeGroup); if (keys.length === 0) { return undefined; } diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts index 357ef7c5..59f051a6 100644 --- a/packages/core/src/types.ts +++ b/packages/core/src/types.ts @@ -97,8 +97,36 @@ export type ZGroup = zarr.Group; // These are used in SDataProps and models, so we keep them accessible from core/types // Re-export Result type and utilities from zarrextra for convenience // Result is used throughout core for explicit error handling -export type { LazyZarrArray, Result, ZAttrsAny, ZarrTree } from 'zarrextra'; -export { ATTRS_KEY, Err, isErr, isOk, Ok, unwrap, unwrapOr, ZARRAY_KEY } from 'zarrextra'; +export type { + LazyZarrArray, + Result, + ZAttrsAny, + ZarrArrayMetadata, + ZarrDataType, + ZarrTree, + ZarrV2ArrayNode, + ZarrV3ArrayNode, +} from 'zarrextra'; +export { + ATTRS_KEY, + Err, + getArrayDtype, + getArrayMetadata, + getChildArray, + getChildGroup, + getChildNode, + getNodeAttrs, + isErr, + isLazyZarrArray, + isOk, + isTextDataType, + isZarrGroup, + normalizeDtype, + Ok, + unwrap, + unwrapOr, + ZARRAY_KEY, +} from 'zarrextra'; /** * Used internally when passing around properties of a spatialdata object to be used by the models/loaders. diff --git a/packages/zarrextra/src/index.ts b/packages/zarrextra/src/index.ts index 2a024a13..ee4340b8 100644 --- a/packages/zarrextra/src/index.ts +++ b/packages/zarrextra/src/index.ts @@ -1,5 +1,6 @@ import * as zarr from 'zarrita'; import { Err, Ok, type Result } from './result'; +import { getArrayMetadata, getNodeAttrs } from './treeNodes'; import type { ConsolidatedStore, LazyZarrArray, @@ -174,11 +175,13 @@ export function serializeZarrTree(obj: ZarrTree | unknown): unknown { const result: Record = {}; - if (ATTRS_KEY in obj && obj[ATTRS_KEY]) { - result._attrs = obj[ATTRS_KEY]; + const attrs = getNodeAttrs(obj); + if (attrs) { + result._attrs = attrs; } - if (ZARRAY_KEY in obj && obj[ZARRAY_KEY]) { - result._zarray = obj[ZARRAY_KEY]; + const arrayMetadata = getArrayMetadata(obj); + if (arrayMetadata) { + result._zarray = arrayMetadata; } for (const key in obj) { @@ -229,11 +232,27 @@ export { export { createPrefixedStore } from './prefixedStore'; export type { Result } from './result'; export { Err, isErr, isOk, Ok, unwrap, unwrapOr } from './result'; +export type { ZarrDataType } from './treeNodes'; +export { + getArrayDtype, + getArrayMetadata, + getChildArray, + getChildGroup, + getChildNode, + getNodeAttrs, + isLazyZarrArray, + isTextDataType, + isZarrGroup, + normalizeDtype, +} from './treeNodes'; export type { ConsolidatedStore, LazyZarrArray, StoreReference, ZAttrsAny, + ZarrArrayMetadata, ZarrTree, + ZarrV2ArrayNode, + ZarrV3ArrayNode, } from './types'; export { ATTRS_KEY, ZARRAY_KEY } from './types'; diff --git a/packages/zarrextra/src/treeNodes.ts b/packages/zarrextra/src/treeNodes.ts new file mode 100644 index 00000000..61402c65 --- /dev/null +++ b/packages/zarrextra/src/treeNodes.ts @@ -0,0 +1,223 @@ +/** + * Runtime discrimination for {@link ZarrTree} nodes. + * + * `ZarrTree`'s index signature admits a group or an array at every key, and a + * `LazyZarrArray` is an object too — so the obvious `typeof node === 'object'` + * test lets an array through, after which its own properties (`get`) read as if + * they were child keys. `zarrita`'s own guards do not apply here: these are + * consolidated-metadata tree nodes, not open `zarr.Array`/`zarr.Group` handles. + * + * The discriminator is {@link ZARRAY_KEY}, required on every array leaf and + * absent from every group. + */ + +import type * as zarr from 'zarrita'; +import type { LazyZarrArray, ZAttrsAny, ZarrArrayMetadata, ZarrTree } from './types'; +import { ATTRS_KEY, ZARRAY_KEY } from './types'; + +/** + * The data types this package can name. + * + * `zarrita`'s own `DataType` is the vocabulary — with `float16` added back, + * because `zarrita` admits that member only when the type environment declares + * `Float16Array`, which an `ES2022` lib does not. The name still appears in real + * stores and is still worth classifying, and where `Float16Array` *is* declared + * this union is exactly `zarr.DataType`. + */ +export type ZarrDataType = zarr.DataType | 'float16'; + +function isPlainNode(node: unknown): node is Record { + return typeof node === 'object' && node !== null && !Array.isArray(node); +} + +/** + * Whether a tree node is an array leaf — something with data behind a `get()`. + */ +export function isLazyZarrArray(node: unknown): node is LazyZarrArray { + return isPlainNode(node) && ZARRAY_KEY in node; +} + +/** + * Whether a tree node is a group — a node whose string keys are children. + * + * This is the complement of {@link isLazyZarrArray} within a tree, so it says + * "not an array leaf" rather than "provably a group": any object that is not an + * array node is walkable as one, which is what callers enumerating children + * need. Pair it with an existence check when the node may be absent. + */ +export function isZarrGroup(node: unknown): node is ZarrTree { + return isPlainNode(node) && !(ZARRAY_KEY in node); +} + +/** + * The attributes of any tree node, group or array, or `undefined` when it has + * none. + */ +export function getNodeAttrs(node: unknown): ZAttrsAny | undefined { + if (!isPlainNode(node)) return undefined; + const attrs = node[ATTRS_KEY]; + return isPlainNode(attrs) ? attrs : undefined; +} + +/** + * The node at `path` below `node`, or `undefined` if any step is missing or a + * step other than the last turns out to be an array. + * + * Own properties only: `getChildNode(tree, '__proto__')` must not walk into + * `Object.prototype` and report it as a group. + */ +export function getChildNode( + node: unknown, + ...path: string[] +): ZarrTree | LazyZarrArray | undefined { + let current: unknown = node; + for (const segment of path) { + if (!isZarrGroup(current) || !Object.hasOwn(current, segment)) return undefined; + current = current[segment]; + } + if (isLazyZarrArray(current)) return current; + return isZarrGroup(current) ? current : undefined; +} + +/** + * The child group at `path`, or `undefined` when it is absent or is an array. + * + * This is the shape most consumers actually want: "the `obs` group of this + * table, if it really is a group". + */ +export function getChildGroup(node: unknown, ...path: string[]): ZarrTree | undefined { + const child = getChildNode(node, ...path); + return isZarrGroup(child) ? child : undefined; +} + +/** + * The child array at `path`, or `undefined` when it is absent or is a group. + */ +export function getChildArray( + node: unknown, + ...path: string[] +): LazyZarrArray | undefined { + const child = getChildNode(node, ...path); + return isLazyZarrArray(child) ? child : undefined; +} + +/** + * The array metadata of a tree node, or `undefined` when the node is not an + * array leaf. + */ +export function getArrayMetadata(node: unknown): ZarrArrayMetadata | undefined { + return isLazyZarrArray(node) ? node[ZARRAY_KEY] : undefined; +} + +/** + * v2 numpy typestrings, minus the endianness prefix, in `zarrita`'s vocabulary. + * + * Mirrors `zarrita`'s own internal `coerceDtype`, which is not exported. The + * point of matching it is that {@link getArrayDtype} and an opened array's + * `dtype` must name the same type for the same store — otherwise a check made + * against tree metadata and the same check made after opening can disagree. + */ +const V2_DTYPE_NAMES: Record = { + b1: 'bool', + i1: 'int8', + u1: 'uint8', + i2: 'int16', + u2: 'uint16', + i4: 'int32', + u4: 'uint32', + i8: 'int64', + u8: 'uint64', + f2: 'float16', + f4: 'float32', + f8: 'float64', +}; + +/** + * v3 data type names, mapped to themselves so a lookup both recognises the name + * and types it — a `Set` would recognise it and leave an assertion behind. + * Unlisted names (`complex64`, the `r*` raw types, extension dtypes) are ones + * `zarrita` cannot read either, so `undefined` is the honest answer. + */ +const V3_DTYPE_NAMES: Record = { + bool: 'bool', + int8: 'int8', + int16: 'int16', + int32: 'int32', + int64: 'int64', + uint8: 'uint8', + uint16: 'uint16', + uint32: 'uint32', + uint64: 'uint64', + float16: 'float16', + float32: 'float32', + float64: 'float64', + string: 'string', +}; + +/** + * Normalise either generation's spelling of a data type to a + * {@link ZarrDataType}, or `undefined` for one we do not model. + * + * Deliberately not a bare string: the answer is meant to be comparable with an + * opened array's `dtype`, so the two layers agree by construction rather than by + * coincidence. + */ +export function normalizeDtype(dtype: string): ZarrDataType | undefined { + const v3 = V3_DTYPE_NAMES[dtype]; + if (v3) return v3; + + // `|O` is the one v2 typestring whose meaning is not in the table below. + if (dtype === '|O') return 'v2:object'; + + const match = /^[<>|=](.+)$/.exec(dtype); + if (!match) return undefined; + const rest = match[1]; + + const named = V2_DTYPE_NAMES[rest]; + if (named) return named; + + // Fixed-width bytes (`S`) and unicode (`U`), which zarrita keeps as-is behind + // a `v2:` prefix because v3 has no equivalent. + const fixedWidth = /^([SU])(\d+)$/.exec(rest); + if (fixedWidth) { + const width = Number(fixedWidth[2]); + return fixedWidth[1] === 'S' ? `v2:S${width}` : `v2:U${width}`; + } + + return undefined; +} + +/** + * The data type of an array node, from consolidated metadata alone — no I/O, and + * without the array having been opened. + * + * The field name differs by generation (`dtype` on v2, `data_type` on v3) and + * both reach the tree, so every consumer that wants a dtype would otherwise have + * to know that. `undefined` for a group, or for a data type we do not model. + */ +export function getArrayDtype(node: unknown): ZarrDataType | undefined { + const metadata = getArrayMetadata(node); + if (!metadata) return undefined; + + const spelling = 'data_type' in metadata ? metadata.data_type : metadata.dtype; + return typeof spelling === 'string' ? normalizeDtype(spelling) : undefined; +} + +/** + * Whether values of this data type are text, and so need decoding to strings. + * + * Covers v3 `string` and v2's fixed-width unicode/bytes *and* `object`. + * `zarrita`'s `isDataType(dtype, 'string')` deliberately excludes `v2:object`, + * which has to be tested separately — testing for one without the other is what + * makes a reader return raw integer codes where labels were expected, with no + * error anywhere. Written once here so both the tree-metadata layer and the + * opened-array layer can ask the same question. + */ +export function isTextDataType(dtype: ZarrDataType): boolean { + return ( + dtype === 'string' || + dtype === 'v2:object' || + dtype.startsWith('v2:U') || + dtype.startsWith('v2:S') + ); +} diff --git a/packages/zarrextra/src/types.ts b/packages/zarrextra/src/types.ts index fa19c493..a4616d84 100644 --- a/packages/zarrextra/src/types.ts +++ b/packages/zarrextra/src/types.ts @@ -28,7 +28,7 @@ export const ZARRAY_KEY = Symbol('.zarray'); */ export type LazyZarrArray = { [ATTRS_KEY]?: ZAttrsAny; - [ZARRAY_KEY]: ZAttrsAny; + [ZARRAY_KEY]: ZarrArrayMetadata; get: () => Promise>; }; @@ -45,35 +45,76 @@ export interface ZarrTree { } /** - * Zarr v3 array node metadata + * Zarr v2 array node metadata, as written to `.zarray`. + * + * `dtype` is a numpy typestring (`; }>; - attributes: Record; - dimension_names: string[]; - zarr_format: number; - node_type: 'array'; - storage_transformers: unknown[]; + attributes?: Record; + dimension_names?: string[]; + zarr_format?: number; + node_type?: 'array'; + storage_transformers?: unknown[]; }; +/** + * The array metadata a tree leaf carries under {@link ZARRAY_KEY} — one of the + * two generations, or an unrecognised record. + * + * The third member is deliberate rather than sloppy. This is unvalidated JSON + * straight from the store, and zarr v3 permits data types we do not model (an + * extension dtype is written as an object, not a string), so a union of only the + * two known shapes would either be a lie or would have to fail the whole store + * open. What the union does buy is the compile error that matters: `dtype` is + * absent from the v3 member and `data_type` from the v2 member, so neither can + * be read without narrowing — reading `.dtype` off a v3 node and silently + * getting `undefined` no longer type-checks. + */ +export type ZarrArrayMetadata = ZarrV2ArrayNode | ZarrV3ArrayNode | ZAttrsAny; + /** * Zarr v3 group node metadata */ diff --git a/packages/zarrextra/tests/treeNodes.spec.ts b/packages/zarrextra/tests/treeNodes.spec.ts new file mode 100644 index 00000000..b9aa1da0 --- /dev/null +++ b/packages/zarrextra/tests/treeNodes.spec.ts @@ -0,0 +1,205 @@ +import { describe, expect, it } from 'vitest'; +import * as zarr from 'zarrita'; +import { + getArrayDtype, + getArrayMetadata, + getChildArray, + getChildGroup, + getChildNode, + getNodeAttrs, + isLazyZarrArray, + isTextDataType, + isZarrGroup, + normalizeDtype, +} from '../src/treeNodes'; +import { ATTRS_KEY, ZARRAY_KEY, type ZarrTree } from '../src/types'; + +/** + * The nodes here are shaped exactly as `openExtraConsolidated` builds them — + * symbol-keyed metadata, a `get()` on leaves — because the whole point of the + * guards is to tell those two apart at runtime, where the type system cannot. + */ +function arrayNode(metadata: Record, attrs?: Record) { + return { + ...(attrs ? { [ATTRS_KEY]: attrs } : {}), + [ZARRAY_KEY]: metadata, + get: () => Promise.reject(new Error('the guards must not touch the data')), + }; +} + +describe('tree node guards', () => { + it('tells an array leaf from a group', () => { + const leaf = arrayNode({ data_type: 'float64' }); + const group = { [ATTRS_KEY]: { 'encoding-type': 'dataframe' }, leiden: leaf }; + + expect(isLazyZarrArray(leaf)).toBe(true); + expect(isZarrGroup(leaf)).toBe(false); + + expect(isZarrGroup(group)).toBe(true); + expect(isLazyZarrArray(group)).toBe(false); + }); + + it('rejects non-nodes rather than reporting them as groups', () => { + for (const value of [undefined, null, 'obs', 42, [], () => {}]) { + expect(isZarrGroup(value)).toBe(false); + expect(isLazyZarrArray(value)).toBe(false); + } + }); + + it('reads attrs off either kind of node', () => { + expect(getNodeAttrs(arrayNode({ dtype: ' { + const index = arrayNode({ data_type: 'string' }); + const tree = { + tables: { + cells: { + [ATTRS_KEY]: { 'encoding-type': 'anndata' }, + obs: { [ATTRS_KEY]: { _index: '_index' }, _index: index }, + }, + }, + } satisfies ZarrTree; + + it('walks a path of groups to the node at the end', () => { + expect(getChildGroup(tree, 'tables', 'cells', 'obs')).toBe(tree.tables.cells.obs); + expect(getChildArray(tree, 'tables', 'cells', 'obs', '_index')).toBe(index); + expect(getChildNode(tree, 'tables', 'cells', 'obs', '_index')).toBe(index); + }); + + it('does not confuse the two kinds', () => { + // The bug the guards exist to stop: an array read as a group, its `get` + // enumerated as if it were a child. + expect(getChildGroup(tree, 'tables', 'cells', 'obs', '_index')).toBeUndefined(); + expect(getChildArray(tree, 'tables', 'cells', 'obs')).toBeUndefined(); + }); + + it('stops at a missing step, and at an array in the middle of a path', () => { + expect(getChildGroup(tree, 'images')).toBeUndefined(); + expect(getChildGroup(tree, 'tables', 'cells', 'obs', '_index', 'anything')).toBeUndefined(); + }); + + it('walks own properties only', () => { + // Otherwise `__proto__` resolves to `Object.prototype`, which has no + // `ZARRAY_KEY` and so would pass for a group. + expect(getChildGroup(tree, '__proto__')).toBeUndefined(); + expect(getChildNode(tree, 'constructor')).toBeUndefined(); + }); +}); + +describe('array metadata and dtype', () => { + it('reads metadata from an array leaf and nothing from a group', () => { + expect(getArrayMetadata(arrayNode({ data_type: 'float64' }))).toEqual({ + data_type: 'float64', + }); + expect(getArrayMetadata({ [ATTRS_KEY]: {} })).toBeUndefined(); + }); + + it('reads both generations’ spelling of the same type', () => { + // `data_type` is v3, `dtype` is v2 — the distinction every consumer would + // otherwise have to know about. + expect(getArrayDtype(arrayNode({ data_type: 'float64' }))).toBe('float64'); + expect(getArrayDtype(arrayNode({ dtype: ' { + expect(getArrayDtype({ [ATTRS_KEY]: { 'encoding-type': 'categorical' } })).toBeUndefined(); + expect(getArrayDtype(arrayNode({}))).toBeUndefined(); + expect(getArrayDtype(arrayNode({ data_type: 'complex64' }))).toBeUndefined(); + // A v3 extension dtype is an object, not a string. + expect(getArrayDtype(arrayNode({ data_type: { name: 'numpy.datetime64' } }))).toBeUndefined(); + }); + + it('normalises v2 typestrings the way zarrita does', () => { + expect(normalizeDtype('u4')).toBe('uint32'); + expect(normalizeDtype(' { + const store = new Map(); + const encoder = new TextEncoder(); + store.set( + '/zarr.json', + encoder.encode(JSON.stringify({ zarr_format: 3, node_type: 'group', attributes: {} })) + ); + const metadata = { + zarr_format: 3, + node_type: 'array', + shape: [2], + data_type: 'float64', + chunk_grid: { name: 'regular', configuration: { chunk_shape: [2] } }, + chunk_key_encoding: { name: 'default', configuration: { separator: '/' } }, + codecs: [{ name: 'bytes', configuration: { endian: 'little' } }], + fill_value: 0, + attributes: {}, + }; + store.set('/scores/zarr.json', encoder.encode(JSON.stringify(metadata))); + + const opened = await zarr.open(zarr.root(store).resolve('/scores'), { kind: 'array' }); + expect(getArrayDtype(arrayNode(metadata))).toBe(opened.dtype); + }); +}); + +describe('isTextDataType', () => { + it('covers every spelling of text, v2 object included', () => { + for (const dtype of ['string', 'v2:object', 'v2:U16', 'v2:S5'] as const) { + expect(isTextDataType(dtype)).toBe(true); + } + }); + + it('is false for everything else', () => { + for (const dtype of ['float64', 'int64', 'bool', 'uint8'] as const) { + expect(isTextDataType(dtype)).toBe(false); + } + }); + + it('matches what an opened array reports for the same values', async () => { + // `v2:object` is the trap: zarrita's own `isDataType(dtype, 'string')` is + // false for it, and testing only for `v2:object` misses v3's `string` — + // either omission renders categorical labels as raw integer codes. + const store = new Map(); + const encoder = new TextEncoder(); + store.set('/.zgroup', encoder.encode(JSON.stringify({ zarr_format: 2 }))); + store.set( + '/categories/.zarray', + encoder.encode( + JSON.stringify({ + zarr_format: 2, + shape: [2], + chunks: [2], + dtype: '|O', + compressor: null, + fill_value: null, + filters: [{ id: 'vlen-utf8' }], + order: 'C', + }) + ) + ); + + const opened = await zarr.open(zarr.root(store).resolve('/categories'), { kind: 'array' }); + expect(opened.dtype).toBe('v2:object'); + expect(isTextDataType(opened.dtype)).toBe(true); + expect(opened.is('string')).toBe(false); + }); +}); From f59649183cb76ee4919944e5db08f9a08a93e2e8 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 18:04:03 +0100 Subject: [PATCH 2/9] Document zarrextra MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The package had a README covering codecs and workers, and nothing anywhere covering the thing `@spatialdata/core` actually consumes it for: the consolidated-metadata tree. Adds a docs section for it. `overview` sets out what the package is for and why it is published outside the `@spatialdata/*` namespace, then covers opening a store, the two halves of `ConsolidatedStore` and which one to reach for, the store extras, and `Result`. Codecs and workers are linked rather than restated — they are already documented in the README, the codec-fixtures page and the worker-bundling pattern, and a second copy would drift. `tree-nodes` covers the node model in depth: why attributes and array metadata hang off symbol keys, why `typeof node === 'object'` is the wrong test and what it costs, the guards and child accessors, and reading a data type from metadata alone. The data-type section explains why the answer is in zarrita's vocabulary rather than a bare string, and calls out `v2:object` — excluded from zarrita's own `isDataType(dtype, 'string')`, and the reason a reader can hand back raw integer codes where labels were expected without raising anything. It ends where zarr stops and AnnData starts: a categorical and a nullable column are both groups, so discriminating group from array is necessary but not sufficient, and the semantics for those live in core. Also corrects the core internals page, which pointed at a `zarrUtils.ts` module and a `tryConsolidated` function that no longer exist. Co-Authored-By: Claude Opus 5 --- docs/docs/core/internals.mdx | 14 +- docs/docs/zarrextra/_category_.json | 8 ++ docs/docs/zarrextra/overview.mdx | 106 ++++++++++++++ docs/docs/zarrextra/tree-nodes.mdx | 206 ++++++++++++++++++++++++++++ packages/zarrextra/README.md | 22 ++- 5 files changed, 351 insertions(+), 5 deletions(-) create mode 100644 docs/docs/zarrextra/_category_.json create mode 100644 docs/docs/zarrextra/overview.mdx create mode 100644 docs/docs/zarrextra/tree-nodes.mdx diff --git a/docs/docs/core/internals.mdx b/docs/docs/core/internals.mdx index f712abfb..28e23a04 100644 --- a/docs/docs/core/internals.mdx +++ b/docs/docs/core/internals.mdx @@ -244,11 +244,17 @@ These are used internally during element construction to validate and type metad ## Store Parsing -The `zarrUtils.ts` module handles parsing zarr store contents: +Parsing zarr store contents lives in `zarrextra`, not in core: -- `tryConsolidated(store)` - Attempts to load consolidated metadata -- `parseStoreContents(store)` - Builds the `ZarrTree` structure -- `serializeZarrTree(tree)` - Serializes for JSON output +- `openExtraConsolidated(source)` - Resolves consolidated metadata and builds the + `ZarrTree`, returning a `Result` +- `serializeZarrTree(tree)` - Serializes for JSON output, converting the symbol-keyed + attributes and array metadata to string keys + +Element construction walks that tree with the guards and accessors described in +[tree nodes](../zarrextra/tree-nodes) — `AbstractElement` narrows an element's own +node to a group once, so no element subclass has to discriminate group from array +itself. ## Result Type Implementation diff --git a/docs/docs/zarrextra/_category_.json b/docs/docs/zarrextra/_category_.json new file mode 100644 index 00000000..3b535280 --- /dev/null +++ b/docs/docs/zarrextra/_category_.json @@ -0,0 +1,8 @@ +{ + "label": "zarrextra", + "position": 3, + "link": { + "type": "generated-index", + "description": "Lower-level zarr helpers used by @spatialdata/core: consolidated-metadata trees, node type guards, data types, codecs and worker-backed chunk decode." + } +} diff --git a/docs/docs/zarrextra/overview.mdx b/docs/docs/zarrextra/overview.mdx new file mode 100644 index 00000000..59b3958c --- /dev/null +++ b/docs/docs/zarrextra/overview.mdx @@ -0,0 +1,106 @@ +--- +sidebar_position: 1 +--- + +# zarrextra Overview + +`zarrextra` is the layer between [`zarrita`](https://github.com/manzt/zarrita.js) and +`@spatialdata/core`. It holds the things a SpatialData reader needs that are not +specific to SpatialData: opening a store and reading its whole hierarchy up front, +navigating that hierarchy, decoding image codecs `zarrita` does not ship, and moving +chunk decode off the main thread. + +It is published outside the `@spatialdata/*` namespace because nothing in it knows +what an image or a table is. If you are reading SpatialData stores, you want +`@spatialdata/core`, which re-exports the parts of this package you are likely to +need. If you are reading some other zarr hierarchy and want the same tree and codec +machinery, you can depend on `zarrextra` alone. + +:::info alpha prerelease + +Versioned alongside the `@spatialdata/*` packages for now, but able to version +independently in future releases. The API described here is not yet stable. + +::: + +```bash +npm install zarrextra +# or +pnpm add zarrextra +``` + +## Opening a store + +`openExtraConsolidated` takes a URL or any `zarrita` `Readable`, resolves consolidated +metadata, and reads the entire hierarchy into a tree — every group, every array, and +every node's attributes — before returning. + +```ts +import { openExtraConsolidated, isErr } from 'zarrextra'; + +const result = await openExtraConsolidated('https://example.com/store.zarr'); +if (isErr(result)) { + console.error(result.error); +} else { + const { zarritaStore, tree } = result.value; +} +``` + +It returns a [`Result`](#result), not a thrown error: failing to open a store is an +ordinary outcome for a URL a user typed, and the message is worth handling rather +than catching. + +The `ConsolidatedStore` it resolves to has two halves, and which one you want depends +on whether you need data or only structure: + +| Field | What it is | Use it for | +|---|---|---| +| `zarritaStore` | `zarr.Listable` — the store, with metadata served from memory | Opening arrays, reading chunks | +| `tree` | `ZarrTree` — the hierarchy as a plain object | Enumerating, navigating, reading metadata without I/O | + +The tree is the reason this package exists. Because every node's attributes and array +metadata are already in memory, a question like "what columns does this table have, +and what type is each one?" is answerable synchronously, before deciding whether any +of it is worth loading. [Tree nodes](./tree-nodes) covers the shape of a node, how to +tell a group from an array, and how to read a data type out of one. + +## Store extras + +Two smaller helpers that come up when a store is not being consumed whole: + +- `createPrefixedStore(store, prefix)` — a `Readable` view rooted at a subpath. Used + to hand a table's own subtree to `anndata.js` without it knowing about SpatialData's + layout above. +- `loadOmeZarrMultiscalesFromStore(store, path)` — reads an OME-Zarr multiscales group + into Viv-compatible pixel sources, reusing an already-open store rather than making + the viewer open its own. + +## Result + +`Result` is a small Rust-style success-or-error union, used wherever a failure +is expected rather than exceptional: + +```ts +type Result = { ok: true; value: T } | { ok: false; error: E }; +``` + +with `Ok`, `Err`, `isOk`, `isErr`, `unwrap` and `unwrapOr`. It is re-exported from +`@spatialdata/core`, so importing it from either package gives the same type. See +[error handling](../core/error-handling) for how core uses it. + +## Codecs and workers + +SpatialData stores in the wild use image codecs `zarrita` does not register by +default, and decoding them on the main thread will stall a render. Those two concerns +are documented where they are used rather than repeated here: + +| Topic | Where | +|---|---| +| Registering JP2K and experimental HTJ2K decode, encoding HTJ2K | [package README](https://github.com/Taylor-CCB-Group/SpatialData.js/blob/main/packages/zarrextra/README.md) | +| Which setup each context needs (Node, browser with vis, browser without) | [codec fixtures](../vis/codec-fixtures) | +| Shipping a worker entry point consumers can use without private URLs | [worker bundling pattern](../worker-bundling) | + +The short version: in Node, call `registerJpeg2kCodec()` / `registerExperimentalHtj2kCodec()` +on the main thread. In a browser using `@spatialdata/vis`, do nothing — the renderer +path enables the bundled codec worker for you. In a browser without vis, call +`enableWorkerChunkDecode()` from `zarrextra/workers` before loading codec-backed data. diff --git a/docs/docs/zarrextra/tree-nodes.mdx b/docs/docs/zarrextra/tree-nodes.mdx new file mode 100644 index 00000000..447e682e --- /dev/null +++ b/docs/docs/zarrextra/tree-nodes.mdx @@ -0,0 +1,206 @@ +--- +sidebar_position: 2 +--- + +# Tree nodes + +`openExtraConsolidated` returns a `ZarrTree`: the store's hierarchy as a plain +JavaScript object, with every node's attributes and array metadata already read into +memory. This page covers what a node looks like, how to tell the two kinds apart, and +how to read a data type out of one without opening anything. + +## The shape of a node + +A tree has two kinds of node, and both are objects: + +```ts +type ZarrTree = { + [ATTRS_KEY]?: ZAttrsAny; + [key: string]: ZarrTree | LazyZarrArray; +}; + +type LazyZarrArray = { + [ATTRS_KEY]?: ZAttrsAny; + [ZARRAY_KEY]: ZarrArrayMetadata; + get: () => Promise>; +}; +``` + +A group's string keys are its children. An array leaf has no children — it has +`get()`, which opens the array when you actually want data. + +Attributes and array metadata hang off **symbol** keys, `ATTRS_KEY` and `ZARRAY_KEY`. +That is deliberate: symbol-keyed properties do not show up in `Object.keys`, +`for...in` or `JSON.stringify`, so enumerating a group's children gives you children +and nothing else. It also means `console.log` of a tree hides them; use +`serializeZarrTree(tree)` to get a plain-string-keyed copy (`_attrs`, `_zarray`) for +debugging. + +## Telling a group from an array + +`ZarrTree`'s index signature admits either kind at every key, so the type alone will +not tell you which one you have. The obvious runtime test is wrong: + +```ts +// Wrong. A LazyZarrArray is an object too. +if (typeof node === 'object') { + for (const columnName of Object.keys(node)) { /* ... 'get' is not a column */ } +} +``` + +An array node slips through, and its own properties are read as if they were child +keys — so a table whose `obs` was somehow an array reports a column called `get`. +Use the guards instead. `ZARRAY_KEY` is the discriminator: required on every array +leaf, absent from every group. + +```ts +import { isLazyZarrArray, isZarrGroup } from 'zarrextra'; + +isLazyZarrArray(node); // node is LazyZarrArray +isZarrGroup(node); // node is ZarrTree +``` + +`zarrita`'s own guards do not apply here — these are consolidated-metadata tree +nodes, not open `zarr.Array` / `zarr.Group` handles. + +:::note + +`isZarrGroup` is the complement of `isLazyZarrArray` *within a tree*: it says "not an +array leaf", not "provably a group". Any object that is not an array node is walkable +as one, which is what a caller enumerating children needs. Pair it with an existence +check when the node might be absent. + +::: + +## Navigating + +Most call sites do not want a guard on its own — they want "the node at this path, if +it is the kind I need". That is three functions: + +```ts +import { getChildNode, getChildGroup, getChildArray } from 'zarrextra'; + +const obs = getChildGroup(tree, 'tables', 'cells', 'obs'); +const index = getChildArray(obs, '_index'); +const either = getChildNode(obs, 'cell_type'); +``` + +Each takes a starting node and a path, and returns `undefined` if any step is +missing, if a step before the last is an array, or if the node at the end is not the +kind asked for. Only own properties are walked, so `getChildGroup(tree, '__proto__')` +is `undefined` rather than `Object.prototype`. + +`getNodeAttrs(node)` reads the attributes off either kind of node, or `undefined` +when it has none: + +```ts +import { getNodeAttrs } from 'zarrextra'; + +const indexName = getNodeAttrs(obs)?._index; // AnnData's dataframe index name +``` + +## Data types + +`getArrayMetadata(node)` returns what the store wrote for an array — but the two zarr +generations spell the data type differently, and both reach the tree: + +- zarr v2 writes `dtype` as a numpy typestring: `` type inspired by Rust for explicit error handling. This is a custom implementation for simplicity and to avoid dependencies. We may review using an existing Result library (such as `neverthrow`) in the future, but for now this provides a lightweight solution. @@ -40,7 +45,22 @@ if (result.ok) { ## API -See the TypeScript definitions for full API documentation. +See [the documentation site](https://taylor-ccb-group.github.io/SpatialData.js/docs/zarrextra/overview) +for the store, tree and data-type APIs, and the TypeScript definitions for everything else. + +The tree returned by `openExtraConsolidated` holds a group or an array at every key, +and both are objects — so use the exported guards rather than a `typeof` test, which +lets an array through and then reads its own `get` property as if it were a child: + +```typescript +import { getArrayDtype, getChildGroup, isLazyZarrArray, isZarrGroup } from 'zarrextra'; + +const obs = getChildGroup(tree, 'tables', 'cells', 'obs'); +const dtype = getArrayDtype(obs?.leiden); // 'float64' | 'string' | ... | undefined +``` + +`getArrayDtype` reads the data type from consolidated metadata with no I/O, folding +zarr v2's numpy typestrings and v3's names into `zarrita`'s own `DataType` vocabulary. ## Codec registration From d09911c8102873d925cc3d5cdf44afc3d1489711 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 18:11:01 +0100 Subject: [PATCH 3/9] Acknowledge zarrextra's weight relative to zarrita MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit zarrita is deliberately minimal and zarrextra is not, so the overview now says where that lands before someone depends on it. The main entry is small (15.4 kB, 5.2 kB gzipped) and the WASM codec packages are optional dependencies injected by the caller, so importing the package does not drag them in — the weight is the codec worker at 3.2 MB, and the fact that it is one bundle carrying every codec, so enabling worker decode for JP2K also ships OpenJPH. Most stores need neither. Also notes zod as a hard dependency reached only by an internal schema module, and flags the packaging rather than the APIs as the part most likely to be redesigned. Co-Authored-By: Claude Opus 5 --- docs/docs/zarrextra/overview.mdx | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/docs/docs/zarrextra/overview.mdx b/docs/docs/zarrextra/overview.mdx index 59b3958c..7f5dc803 100644 --- a/docs/docs/zarrextra/overview.mdx +++ b/docs/docs/zarrextra/overview.mdx @@ -104,3 +104,35 @@ The short version: in Node, call `registerJpeg2kCodec()` / `registerExperimental on the main thread. In a browser using `@spatialdata/vis`, do nothing — the renderer path enables the bundled codec worker for you. In a browser without vis, call `enableWorkerChunkDecode()` from `zarrextra/workers` before loading codec-backed data. + +## Weight + +`zarrita` is deliberately minimal. `zarrextra` is not, and it is worth being explicit +about where that lands before depending on it. + +Measured from the current build: + +| Entry | Raw | Gzipped | +|---|---|---| +| `zarrextra` | 15.4 kB | 5.2 kB | +| `zarrextra/codec-worker` | 3.2 MB | 899 kB | + +The main entry is small, and the WASM codec packages are `optionalDependencies` whose +decoders are injected by the caller — so importing `zarrextra` does not drag +OpenJPEG or OpenJPH in. What is heavy is the codec worker, and it is one bundle +carrying every codec: enabling worker decode for JP2K also ships OpenJPH, and vice +versa. An application reading uncompressed or blosc-compressed stores — which is most +of them — gets no benefit from either. + +The install footprint is also larger than the import graph suggests. `zod` is a hard +dependency used by a single internal schema module, so every consumer installs it +whether or not anything reaches it. + +:::caution subject to redesign + +None of this is settled. Splitting the worker so applications can opt into lighter +bundles when they do not need JP2K or HTJ2K is the obvious first move, and is already +noted as future work in the package README. Treat the packaging — not the APIs on +this page — as the part most likely to change. + +::: From 79d68cd2c3ab4d8d86f9155b9903e631c946f2ad Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 18:38:01 +0100 Subject: [PATCH 4/9] Guard dtype and encoding lookups against inherited properties MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both tables are object literals indexed with a name that came out of a store, so `constructor` is as possible an input as `float64`. Unguarded, the lookup answers with `Object` — truthy, so it escapes as if it were a real value. For `normalizeDtype` that is worse than a wrong answer: `getArrayDtype` hands back a function, and the next thing core does with it is `isTextDataType`, which throws `TypeError: dtype.startsWith is not a function`. Classifying a column reports "unknown" for every dtype it does not model; it should not crash for this one. `OBS_KIND_BY_ENCODING` in core has the same shape and returns `Object` as a `TableColumnKind`. `Object.hasOwn` on all three lookups, matching what `getChildNode` already does for the same reason one function up. Regression tests cover both packages and fail without the fix with exactly those two symptoms. Also balances the docs note on zod: array metadata is read with a bare JSON.parse and typed as an unvalidated record, so validating more at that boundary is at least as live an option as dropping the dependency. Co-Authored-By: Claude Opus 5 --- docs/docs/zarrextra/overview.mdx | 8 ++++++-- packages/core/src/models/index.ts | 5 ++++- packages/core/tests/tableElement.spec.ts | 18 ++++++++++++++++++ packages/zarrextra/src/treeNodes.ts | 11 +++++++---- packages/zarrextra/tests/treeNodes.spec.ts | 12 ++++++++++++ 5 files changed, 47 insertions(+), 7 deletions(-) diff --git a/docs/docs/zarrextra/overview.mdx b/docs/docs/zarrextra/overview.mdx index 7f5dc803..91735d2b 100644 --- a/docs/docs/zarrextra/overview.mdx +++ b/docs/docs/zarrextra/overview.mdx @@ -125,8 +125,12 @@ versa. An application reading uncompressed or blosc-compressed stores — which of them — gets no benefit from either. The install footprint is also larger than the import graph suggests. `zod` is a hard -dependency used by a single internal schema module, so every consumer installs it -whether or not anything reaches it. +dependency reached by a single internal schema module that nothing currently calls, +so every consumer installs it whether or not anything uses it. Which way that should +resolve is open: array metadata is read from a store with a bare `JSON.parse` today +and typed as [an unvalidated record](./tree-nodes#array-metadata-types), so there is +at least as good a case for validating more at that boundary — and earning the +dependency — as for dropping it. :::caution subject to redesign diff --git a/packages/core/src/models/index.ts b/packages/core/src/models/index.ts index 340b0655..0fd2949c 100644 --- a/packages/core/src/models/index.ts +++ b/packages/core/src/models/index.ts @@ -271,7 +271,10 @@ function classifyObsDtype(dtype: ZarrDataType): TableColumnKind { export function classifyObsColumnNode(node: unknown): TableColumnKind | undefined { const attrs = getNodeAttrs(node); const encoding = attrs?.['encoding-type']; - if (typeof encoding === 'string' && OBS_KIND_BY_ENCODING[encoding]) { + // Own properties only: the encoding name came out of a store, so it can be + // `constructor` as easily as `categorical`, and an unguarded lookup would + // answer that with `Object` and return it as if it were a column kind. + if (typeof encoding === 'string' && Object.hasOwn(OBS_KIND_BY_ENCODING, encoding)) { return OBS_KIND_BY_ENCODING[encoding]; } // Older AnnData writes a `categories` attribute pointing at the levels instead diff --git a/packages/core/tests/tableElement.spec.ts b/packages/core/tests/tableElement.spec.ts index a12c665f..84b66548 100644 --- a/packages/core/tests/tableElement.spec.ts +++ b/packages/core/tests/tableElement.spec.ts @@ -204,4 +204,22 @@ describe('obs column kinds from consolidated metadata', () => { expect(table.getObsColumnKinds(['mystery', 'not_there'])).toEqual([undefined, undefined]); }); + + it('does not mistake an inherited property of its encoding table for a kind', () => { + // `encoding-type` and the dtype both come out of a store, so either can name + // something on `Object.prototype`. Unguarded lookups answer with `Object`, + // which is truthy: the encoding table returns it as a column kind, and the + // dtype table hands the classifier a function that then throws. + const table = tableWithObs({ + by_encoding: { [ATTRS_KEY]: { 'encoding-type': 'constructor' } }, + by_dtype: arrayNode({ data_type: 'constructor' }), + by_typestring: arrayNode({ dtype: ' = { * Deliberately not a bare string: the answer is meant to be comparable with an * opened array's `dtype`, so the two layers agree by construction rather than by * coincidence. + * + * Own properties only, in both tables. The name being looked up came out of a + * store's metadata, so `constructor` is as possible as `float64` — and an + * unguarded lookup answers it with `Object`, which is truthy, escapes as if it + * were a data type, and makes the next `dtype.startsWith` throw. */ export function normalizeDtype(dtype: string): ZarrDataType | undefined { - const v3 = V3_DTYPE_NAMES[dtype]; - if (v3) return v3; + if (Object.hasOwn(V3_DTYPE_NAMES, dtype)) return V3_DTYPE_NAMES[dtype]; // `|O` is the one v2 typestring whose meaning is not in the table below. if (dtype === '|O') return 'v2:object'; @@ -173,8 +177,7 @@ export function normalizeDtype(dtype: string): ZarrDataType | undefined { if (!match) return undefined; const rest = match[1]; - const named = V2_DTYPE_NAMES[rest]; - if (named) return named; + if (Object.hasOwn(V2_DTYPE_NAMES, rest)) return V2_DTYPE_NAMES[rest]; // Fixed-width bytes (`S`) and unicode (`U`), which zarrita keeps as-is behind // a `v2:` prefix because v3 has no equivalent. diff --git a/packages/zarrextra/tests/treeNodes.spec.ts b/packages/zarrextra/tests/treeNodes.spec.ts index b9aa1da0..d0238eff 100644 --- a/packages/zarrextra/tests/treeNodes.spec.ts +++ b/packages/zarrextra/tests/treeNodes.spec.ts @@ -131,6 +131,18 @@ describe('array metadata and dtype', () => { expect(normalizeDtype('nonsense')).toBeUndefined(); }); + it('does not answer with inherited properties of its lookup tables', () => { + // The name comes out of a store, so it can be anything. An unguarded + // `TABLE[name]` answers `constructor` with `Object` — truthy, so it escapes + // as if it were a data type, and the next `dtype.startsWith` throws. + for (const inherited of ['constructor', 'toString', 'hasOwnProperty', '__proto__']) { + expect(normalizeDtype(inherited)).toBeUndefined(); + expect(normalizeDtype(`<${inherited}`)).toBeUndefined(); + expect(getArrayDtype(arrayNode({ data_type: inherited }))).toBeUndefined(); + expect(getArrayDtype(arrayNode({ dtype: `<${inherited}` }))).toBeUndefined(); + } + }); + /** * The reason to normalise into zarrita's vocabulary at all: a dtype read from * tree metadata and one read from an opened array have to be the same value, From 1660bd29873ccacaf800044feaafbcc0020c0487 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 18:50:21 +0100 Subject: [PATCH 5/9] Resolve sibling packages to source in unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `vitest.config.ts` lists the packages as projects so each runs under its own `vite.config.ts`, and only the integration project aliased workspace packages to their sources. Every package project therefore resolved them the normal way: through the workspace link, to `dist`. So a test in `core` importing `zarrextra` was testing whatever was last built. Editing `packages/zarrextra/src` changed nothing until a rebuild, and the suite went on passing — or failing — against the previous build with no hint that the source under the cursor was not the source under test. That is exactly how the guard added in 79d68cd appeared not to work: the fix was in `src`, the assertion was running against `dist`, and the stack trace pointed at `src` anyway because the bundle carries a sourcemap. `vis` already avoided this with `createWorkspaceSourceAliases`. Applies the same helper in `core` and `layers`, which are the other two packages whose tests import a sibling by name, and points the integration project at the helper too rather than a second hand-maintained list that was already missing entries. Harmless for `build`: rollup consults `external` with the unresolved specifier, so the alias never applies there. Verified after the change — `core`'s bundle still imports `from "zarrextra"` and inlines none of it, and `layers` still imports `from "@spatialdata/core"`. Verified the gap is actually closed by breaking `zarrextra/src` with `dist` left intact: `core`'s test now fails, where before it passed. Co-Authored-By: Claude Opus 5 --- packages/core/vite.config.ts | 14 ++++++++++++++ packages/layers/vite.config.ts | 6 ++++++ vitest.config.ts | 10 +++++----- 3 files changed, 25 insertions(+), 5 deletions(-) diff --git a/packages/core/vite.config.ts b/packages/core/vite.config.ts index ce221f35..2181fd48 100644 --- a/packages/core/vite.config.ts +++ b/packages/core/vite.config.ts @@ -1,10 +1,24 @@ import { cpSync } from 'node:fs'; import { resolve } from 'node:path'; import { defineConfig } from 'vitest/config'; +import { createWorkspaceSourceAliases } from '../../vite.config.base'; const rollupExternals = new Set(['zarrita', 'zod', 'anndata.js', 'zarrextra', 'apache-arrow']); export default defineConfig({ + // Resolve sibling packages to their sources, as `vis` already does. + // + // Without this, a test here importing `zarrextra` gets whatever is in that + // package's `dist` — so editing `packages/zarrextra/src` changes nothing until + // it is rebuilt, and the suite silently keeps testing the previous build. The + // failure is invisible: tests pass or fail against stale code with no hint + // that the source under the cursor is not the source under test. + // + // Harmless for `build`: rollup consults `external` with the unresolved + // specifier, so `zarrextra` is externalized before an alias could apply. + resolve: { + alias: createWorkspaceSourceAliases(resolve(__dirname, '../..')), + }, build: { lib: { entry: { diff --git a/packages/layers/vite.config.ts b/packages/layers/vite.config.ts index c81afbfd..bc3b09c3 100644 --- a/packages/layers/vite.config.ts +++ b/packages/layers/vite.config.ts @@ -1,9 +1,15 @@ import { resolve } from 'node:path'; import { defineConfig } from 'vitest/config'; +import { createWorkspaceSourceAliases } from '../../vite.config.base'; // .d.ts files are emitted via `tsc --emitDeclarationOnly` in the build script. export default defineConfig({ root: resolve(__dirname), + // Sibling packages resolve to their sources — see the note in + // `packages/core/vite.config.ts` for why the default is a trap for tests. + resolve: { + alias: createWorkspaceSourceAliases(resolve(__dirname, '../..')), + }, build: { outDir: resolve(__dirname, 'dist'), lib: { diff --git a/vitest.config.ts b/vitest.config.ts index bf6394cf..aaf03769 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,6 +1,7 @@ import { defineConfig } from 'vitest/config'; -import { dirname, resolve } from 'node:path'; +import { dirname } from 'node:path'; import { fileURLToPath } from 'node:url'; +import { createWorkspaceSourceAliases } from './vite.config.base'; const __filename = fileURLToPath(import.meta.url); // not strictly necessary as vite will provide this in config context @@ -36,10 +37,9 @@ export default defineConfig({ hookTimeout: 60000, }, resolve: { - alias: { - '@spatialdata/core': resolve(__dirname, 'packages/core/src'), - zarrextra: resolve(__dirname, 'packages/zarrextra/src'), - }, + // The same source aliases the package projects use, rather than a + // second hand-maintained list that was already missing entries. + alias: createWorkspaceSourceAliases(__dirname), }, }, ], From 8caa2a9cf292cc023af506ca374a2bb2fe18e3b0 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 19:05:16 +0100 Subject: [PATCH 6/9] Alias sibling sources in react and avivatorish too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completes the previous commit. Neither package imports a sibling from a test today, so this changes nothing now — the point is that the first test which does should not be the one that has to discover the trap. The reason these two were held back was that the alias sits in `resolve` and so applies to `build` as well, and both are built by `dev` (`vite build --watch`). That turns out to be safe for the same reason it was in `core` and `layers`: each externalizes exactly the sibling specifier it imports — `@spatialdata/core` for react, `zarrextra` for avivatorish — and rollup consults `external` with the unresolved specifier, so the alias never fires. Confirmed by checksum: both bundles are byte-identical before and after. Confirmed the aliases do take effect where they are meant to, by adding a probe export to `zarrextra/src` and `core/src` that no `dist` contained and asserting on it from a test in each package. Caveat for later: a string entry in `external` matches the exact specifier only, so importing a *subpath* — `zarrextra/workers` — without adding it there would let the alias inline that source into the bundle. Neither package does today. Co-Authored-By: Claude Opus 5 --- packages/avivatorish/vite.config.ts | 11 ++++++++++- packages/react/vite.config.ts | 11 ++++++++++- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/avivatorish/vite.config.ts b/packages/avivatorish/vite.config.ts index 64e99744..55ccc580 100644 --- a/packages/avivatorish/vite.config.ts +++ b/packages/avivatorish/vite.config.ts @@ -1,8 +1,10 @@ +import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { defineViteConfig } from '../../vite.config.base'; +import { createWorkspaceSourceAliases, defineViteConfig } from '../../vite.config.base'; import { mergeConfig } from 'vite'; const pkgRoot = fileURLToPath(new URL('.', import.meta.url)); +const workspaceRoot = path.resolve(pkgRoot, '../..'); const baseConfig = defineViteConfig({ pkgRoot, @@ -11,6 +13,13 @@ const baseConfig = defineViteConfig({ }); export default mergeConfig(baseConfig, { + // Sibling packages resolve to their sources — see the note in + // `packages/core/vite.config.ts` for why the default is a trap for tests. + // Nothing here imports a sibling in a test yet; this is so the first one that + // does is not the one that has to discover it. + resolve: { + alias: createWorkspaceSourceAliases(workspaceRoot), + }, test: { globals: true, environment: 'jsdom', diff --git a/packages/react/vite.config.ts b/packages/react/vite.config.ts index 6c248487..362ea34d 100644 --- a/packages/react/vite.config.ts +++ b/packages/react/vite.config.ts @@ -1,8 +1,10 @@ +import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { mergeConfig } from 'vite'; -import { defineViteConfig } from '../../vite.config.base'; +import { createWorkspaceSourceAliases, defineViteConfig } from '../../vite.config.base'; const pkgRoot = fileURLToPath(new URL('.', import.meta.url)); +const workspaceRoot = path.resolve(pkgRoot, '../..'); const baseConfig = defineViteConfig({ pkgRoot, @@ -12,6 +14,13 @@ const baseConfig = defineViteConfig({ }); export default mergeConfig(baseConfig, { + // Sibling packages resolve to their sources — see the note in + // `packages/core/vite.config.ts` for why the default is a trap for tests. + // Nothing here imports a sibling in a test yet; this is so the first one that + // does is not the one that has to discover it. + resolve: { + alias: createWorkspaceSourceAliases(workspaceRoot), + }, test: { globals: true, environment: 'jsdom', From fb79fdde6d31584c57cc00ea728c97fc853ac042 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 19:13:52 +0100 Subject: [PATCH 7/9] Externalize zarrextra subpaths in avivatorish MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the caveat noted in the previous commit. A string entry in `external` matches the exact specifier only, so `zarrextra/workers` would have been left to resolve — and with the workspace source alias now in this config, resolving it means inlining that source into the bundle. The regex form matches what `vis` already uses. Verified both halves rather than assuming: with a `zarrextra/workers` import added to the package's source, the regex keeps it external and inlines none of it, and reverting to the string entry inlines it. The bundle is byte-identical before and after the change as things stand, since nothing imports a subpath yet. Also takes biome's import-order fix while here — pre-existing, and unflagged because the `lint:biome` gate only covers `packages/*/src`. Co-Authored-By: Claude Opus 5 --- packages/avivatorish/vite.config.ts | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/avivatorish/vite.config.ts b/packages/avivatorish/vite.config.ts index 55ccc580..65df243e 100644 --- a/packages/avivatorish/vite.config.ts +++ b/packages/avivatorish/vite.config.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import { fileURLToPath } from 'node:url'; -import { createWorkspaceSourceAliases, defineViteConfig } from '../../vite.config.base'; import { mergeConfig } from 'vite'; +import { createWorkspaceSourceAliases, defineViteConfig } from '../../vite.config.base'; const pkgRoot = fileURLToPath(new URL('.', import.meta.url)); const workspaceRoot = path.resolve(pkgRoot, '../..'); @@ -9,7 +9,17 @@ const workspaceRoot = path.resolve(pkgRoot, '../..'); const baseConfig = defineViteConfig({ pkgRoot, libName: 'SpatialDataAvivatorish', - external: ['@hms-dbmi/viv', '@math.gl/core', 'geotiff', 'zarrita', 'zarrextra', /^zustand(?:\/.*)?$/], + external: [ + '@hms-dbmi/viv', + '@math.gl/core', + 'geotiff', + 'zarrita', + // Subpaths too (`zarrextra/workers`): a string entry matches the exact + // specifier only, which would leave a subpath import to be resolved — and + // the workspace source alias below would then inline it into the bundle. + /^zarrextra(?:\/.*)?$/, + /^zustand(?:\/.*)?$/, + ], }); export default mergeConfig(baseConfig, { From d2524a551a8b6f10102ae92c0b0581ae61dd9bbb Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 19:18:22 +0100 Subject: [PATCH 8/9] Externalize dependency subpaths in core MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The same change as the previous commit, for the one config that externalizes by function rather than by list. `rollupExternals.has(id)` matched the exact specifier only, so a subpath entry point — `zarrextra/workers`, `zod/v4` — would have been resolved and bundled, and with the workspace source aliases now in this config, resolving `zarrextra/workers` means inlining a sibling's source. Spelled as a predicate rather than an array of regexes because the surrounding `external` is already a function; the matching is what `/^name(?:\/.*)?$/` means. Byte-neutral today. `apache-arrow/vector` is the only subpath core imports and both imports are `import type`, so it is erased before rollup sees it — the bundle is identical before and after. Verified the change bites by making that import a value import: the predicate keeps the specifier external, while the exact-match form resolves it into a chunk. Leaves alone, as a separate question: `@math.gl/core`, `earcut` and `ol` are declared runtime dependencies that this config does not externalize at all, so they are bundled into `dist` today. That may well be deliberate. Co-Authored-By: Claude Opus 5 --- packages/core/vite.config.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/core/vite.config.ts b/packages/core/vite.config.ts index 2181fd48..71472b96 100644 --- a/packages/core/vite.config.ts +++ b/packages/core/vite.config.ts @@ -3,7 +3,22 @@ import { resolve } from 'node:path'; import { defineConfig } from 'vitest/config'; import { createWorkspaceSourceAliases } from '../../vite.config.base'; -const rollupExternals = new Set(['zarrita', 'zod', 'anndata.js', 'zarrextra', 'apache-arrow']); +const rollupExternals = ['zarrita', 'zod', 'anndata.js', 'zarrextra', 'apache-arrow']; + +/** + * Left for the consumer to resolve — the package itself, and any subpath of it. + * + * Equivalent to the `/^name(?:\/.*)?$/` form the other packages pass as regexes, + * spelled as a predicate because this config externalizes by function. Matching + * the exact specifier alone would leave a subpath entry point — `zarrextra/workers`, + * `zod/v4` — to be resolved and bundled, and with the workspace source aliases + * above, resolving `zarrextra/workers` means inlining a sibling's source. + * + * `apache-arrow/vector` is imported here already, but only as a type, so it is + * erased before rollup sees it. The first value import of a subpath would not be. + */ +const isExternalPackage = (id: string) => + rollupExternals.some((name) => id === name || id.startsWith(`${name}/`)); export default defineConfig({ // Resolve sibling packages to their sources, as `vis` already does. @@ -41,7 +56,7 @@ export default defineConfig({ if (normalizedId.includes('vendor/parquet-wasm/parquet_wasm.js')) { return true; } - return rollupExternals.has(id); + return isExternalPackage(id); }, }, sourcemap: true, From 2c40e989a46245c2c320cb9b4442e7a341b3aff9 Mon Sep 17 00:00:00 2001 From: Peter Todd Date: Fri, 31 Jul 2026 19:35:36 +0100 Subject: [PATCH 9/9] Externalize core's remaining runtime dependencies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `@math.gl/core`, `earcut` and `ol` were declared `dependencies` that the build did not externalize, so they were bundled into `dist`. That is the arrangement that puts two copies of a library in one application: `@math.gl/core` is also a direct dependency of `layers`, `vis` and `avivatorish`, and `Matrix4` instances have to survive being passed between them. All three are ordinary `dependencies`, so consumers install them transitively and need to do nothing. `ol` is reached only as `ol/format/WKB.js`, so it is external only by way of the subpath matching added in the previous commit — a plain entry would not have matched it. The bundle gets meaningfully smaller: `index.js` 174.6 kB -> 149.5 kB, and the tessellation chunk 86.2 kB -> 6.7 kB, which was almost entirely earcut and the OpenLayers WKB parser. Checked the worker entry first, since it is the one output where this could break at runtime: `points-worker.js` is loaded with `new Worker(new URL(...))` straight from disk, with no consumer bundler in the path, so a bare specifier there would be unresolvable. Its dependency graph does not reach any of the three — it still imports nothing, and is byte-identical. Verified end to end with `tests/production/browser`, which builds a consumer app against `dist` and renders polygon shapes — the exact path through earcut and the WKB decoder. It builds and passes. Co-Authored-By: Claude Opus 5 --- packages/core/vite.config.ts | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/packages/core/vite.config.ts b/packages/core/vite.config.ts index 71472b96..50cb3fa4 100644 --- a/packages/core/vite.config.ts +++ b/packages/core/vite.config.ts @@ -3,7 +3,28 @@ import { resolve } from 'node:path'; import { defineConfig } from 'vitest/config'; import { createWorkspaceSourceAliases } from '../../vite.config.base'; -const rollupExternals = ['zarrita', 'zod', 'anndata.js', 'zarrextra', 'apache-arrow']; +/** + * Every runtime dependency this package declares — all of them `dependencies`, + * so a consumer installs them transitively and needs to do nothing. + * + * The list is the whole set on purpose. Externalizing some and bundling others + * is the arrangement that produces two copies of a library in one application: + * `@math.gl/core` in particular is also a direct dependency of `layers` and + * `vis`, and `Matrix4` instances have to survive being passed between them. + * + * `ol` is only ever reached as `ol/format/WKB.js` — a subpath, and so external + * only by way of the matching below. + */ +const rollupExternals = [ + 'zarrita', + 'zod', + 'anndata.js', + 'zarrextra', + 'apache-arrow', + '@math.gl/core', + 'earcut', + 'ol', +]; /** * Left for the consumer to resolve — the package itself, and any subpath of it.