Add unit tests, modernize tooling and release 7.0.0 (ESM output) by v-aidaba · Pull Request #11 · microsoft/powerbi-visuals-utils-onobjectutils

v-aidaba · 2026-08-20T06:22:01Z

Add unit tests, modernize tooling and release 7.0.0

Aligns the package with the other powerbi-visuals-utils v7 repositories:
adds a unit test suite, migrates the lint/build tooling and makes CI
actually verify the code.

Tests:

migrated to Vitest 4 with the jsdom environment, 80 tests, 96% coverage
powerbi-visuals-api ships ambient const enums with no runtime
representation, so a mock is aliased in place of the package while
testing
added test/tsconfig.json and a test:typecheck script

Tooling:

TypeScript: ES2020 output with bundler resolution, strict, skipLibCheck
ESLint: flat config, ESLint 9, typescript-eslint 8; tests are linted too
added the missing typescript dev dependency
scripts: prebuild, prepublishOnly, test, test:watch, test:coverage,
test:typecheck, lint, lint:fix
pinned jsdom to ^29.1.1, the last line supporting Node 20

Packaging:

explicit files whitelist, sideEffects: false, keywords, author
dropped the legacy jsnext:main

CI:

build.yml now runs audit, lint, typecheck, tests and build on Node 20
and 22; previously only lint ran
modernized release.yml and codeql-analysis.yml, added Dependabot

Fixes:

createVisualSubSelectionForSingleObject matched the sub-selection type
with the in operator, which tests array indices instead of values, so
the selection origin offset was never applied to NumericText and was
wrongly applied to None
getDataForElement is now typed as SubSelectionElementData | null
ISubSelectionHelper.getAllSubSelectables now declares the filterType
argument the implementation already accepted

BREAKING CHANGE: the package is published as ES2020 modules instead of
CommonJS, so a direct require() from Node is no longer supported; bundler
based consumption (webpack/pbiviz) is unaffected.

BREAKING CHANGE: powerbi-visuals-api moved to devDependencies and is no
longer installed transitively.

Alexey (Demonkratiy)

Ran the whole CI pipeline locally on this branch: lint, test:typecheck, test:coverage (80 tests, 96.3% statements / 92.1% branches), build, and npm audit --audit-level=high --omit=dev — all green. Lock file is clean too (no resolved outside registry.npmjs.org, no sha1- integrity).

Notes are inline.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

    }

-    public getAllSubSelectables(filterType?: SubSelectionStylesType): CustomVisualSubSelection[] | undefined {
+    public getAllSubSelectables(filterType?: SubSelectionStylesType): CustomVisualSubSelection[] {


Same falsy-guard class of bug as the in fix in this PR, ~10 lines below:

if (filterType) {

SubSelectionStylesType.None === 0, so getAllSubSelectables(SubSelectionStylesType.None) skips the filter entirely and silently returns every sub-selectable instead of only the None-typed ones.

The rest of the path is fine: data-sub-selection-type="0" is read correctly by getSubSelectionTypeFromElement (the string "0" is truthy), and Number(type) === filterType also works for 0. The guard is the only broken part.

Verified with a throwaway spec on three elements (None/Shape/Text):

byShape: [ 'shape' ] byNone : [ 'none', 'shape', 'text' ] // expected [ 'none' ]

if (filterType !== undefined) fixes it, and the spec goes green. Pre-existing, but squarely in scope: this PR fixes the in bug that misfires on exactly the same zero value, and adds a filterType test that can't catch this one.

Good catch, fixed. Changed to if (filterType !== undefined). Added a test with None/Shape/Text elements asserting that getAllSubSelectables(SubSelectionStylesType.None) returns only the None one — it fails against the old guard and passes now.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

        const objectName = d3.select(subSelectionElement).attr(SubSelectableObjectNameAttribute);
        const displayName = this.getDisplayNameFromElement(subSelectionElement);
-        const subSelectionType = this.getSubSelectionTypeFromElement(subSelectionElement);
+        const subSelectionType = this.getSubSelectionTypeFromElement(subSelectionElement) as SubSelectionStylesType;


getSubSelectionTypeFromElement returns SubSelectionStylesType | undefined by design (line ~798 — the attribute is missing). This cast erases that and lets subSelectionType: undefined land inside a CustomVisualSubSelection, where the API declares the field non-optional. Same cast at line 773.

data-sub-selection-type is documented as required in the README but nothing enforces it, so an attribute-less element is reachable — and your own tests create them (createSubSelectable(host, { objectName: "bottom", displayName: "Bottom", rect: ... }) with no subSelectionType), then feed them through getAllSubSelectables.

Either default it (?? SubSelectionStylesType.None) or propagate the | undefined, but please don't cast it away: this is exactly the case strict was turned on to catch.

Agreed, and I went with the default rather than propagating | undefined, since the API declares the field as required. Both sites (getCreateVisualSubSelectionArgs and createSubSelectionFromElement) now use ?? SubSelectionStylesType.None and the as SubSelectionStylesType casts are gone, so the compiler enforces this instead of us asserting it away. Added a test with an attribute-less element asserting subSelectionType === None.

Follow-up on the same method, since ?? alone didn't fully close it: Number(type) returns NaN for a non-numeric attribute (e.g. data-sub-selection-type="shape"), and NaN is neither null nor undefined, so it survived the ?? and was still typed as SubSelectionStylesType on the way into CustomVisualSubSelection. getSubSelectionTypeFromElement now validates the parsed value against the known enum members and returns undefined for anything else, which then defaults to None on the same path as a missing attribute. Reverse mapping isn't available here — SubSelectionStylesType is a const enum with no runtime object , hence the explicit member list.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

            return;
        }

+        const currentSubSelections = subSelections;


This alias is dead code — the parameter narrows fine on its own inside the closure. I removed the line, pointed the call below back at subSelections, and npm run test:typecheck still exits 0.

Please just drop it.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

+        const origin = useOffsetInSelection ? { ...selectionOrigin, offset: { x: 0, y: selectionOrigin.y * -1 } } : selectionOrigin;
        const visualSubSelection: CustomVisualSubSelection = {
-            customVisualObjects: [{ objectName, selectionId: selectionId ?? undefined }],
+            customVisualObjects: [{ objectName, selectionId: (selectionId ?? undefined) as ISelectionId }],


These two casts ((selectionId ?? undefined) as ISelectionId, and origin as CustomVisualSubSelection["selectionOrigin"] on line 756) encode a real mismatch rather than a compiler quirk. The API declares both as required:

export interface CustomVisualObject { objectName: string; selectionId: powerbi.visuals.ISelectionId; // required } export interface CustomVisualSubSelection { selectionOrigin: SubSelectionOrigin; // required ... }

while CreateVisualSubSelectionFromObjectArgs declares them optional and this method genuinely emits undefined — your own test "omits optional members when they are not provided" asserts exactly that. Same story with subSelect(undefined as unknown as CustomVisualSubSelection) on line 168, where subSelect(subSelection: CustomVisualSubSelection) takes a required parameter.

I'm not asking you to change the API or the runtime behaviour in this PR — both are out of scope for a tooling release. Just add a one-line comment per cast naming which API type is inaccurate, so the next reader doesn't have to re-derive it from the .d.ts. As written they read as "silence the compiler".

Separate question while I'm here: focusOrder does not exist in powerbi-visuals-api@5.11.1 at all — I grepped every src/*.d.ts, zero hits. It only compiles because the conditional spread ...focusOrder ? { focusOrder } : {} bypasses excess-property checking. Are the API types behind the host here, or is the field being dropped on the floor?

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

 }

-export function isArrayEmpty(array: any[]): boolean {
+export function isArrayEmpty<T>(array: T[] | undefined | null): array is undefined | null | [] {


The [] in the predicate is an empty tuple, which is unusual enough to deserve a one-line comment — it's what makes the false branch narrow to T[], and therefore what makes !isArrayEmpty(...) work at the call sites. Without a note it looks like a typo.

Also: this changes the signature of an exported function (any[] -> generic + type predicate). Source-compatible, but it does change inference for consumers, and it isn't mentioned in the CHANGELOG.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

+        },
+    },
+    test: {
+        environment: "jsdom",


Please migrate to browser mode instead of jsdom, to match the rest of the v7 family — svgutils and chartutils both run @vitest/browser + playwright.

The reason matters here specifically: a large part of this helper is layout geometry, and under jsdom getBoundingClientRect returns zeros, so the tests have to hand-stub it (setRect in test/testUtils.ts). Clamp/clip and outline rects are therefore validated against a synthetic layout that cannot disagree with the implementation. Real browser layout is what this code actually runs against.

What it takes:

@vitest/browser + @vitest/browser-playwright + playwright in devDependencies, drop jsdom

test: { browser: { enabled: true, provider: 'playwright', instances: [{ browser: 'chromium' }] } }

- run: npx playwright install --with-deps chromium before the test step — note this touches both workflows, svgutils and chartutils each have that step in release.yml too, not just build.yml

If you'd rather keep jsdom for this package, that's a reasonable conversation to have — but then please say why in the PR description, because right now it silently diverges from the family.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

+            provider: "v8",
+            reporter: ["text", "html", "lcov"],
+            include: ["src/**/*.ts"],
+            exclude: ["src/index.ts", "src/types.ts"],


No coverage.thresholds, so the 96% the PR advertises isn't enforced anywhere and CI won't notice a regression. Something like thresholds: { statements: 95, branches: 90, functions: 95, lines: 95 } would lock in what you achieved.

Minor, a few lines up: the test/**/*{Test,Tests}.ts include pattern matches nothing in this repo — leftover from the Karma naming convention. The *.{test,spec}.ts line below covers everything.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

+    Clip: 1,
+} as const;
+
+export const SubSelectionStylesType = {


The mock is unavoidable and I have no objection to the approach — powerbi-visuals-api's index.js only exports version and schemas, and these are ambient const enums that esbuild cannot inline. Worth keeping the comment you already wrote.

The remaining risk is that the numbers are hand-copied: if the API ever renumbers a member, the tests stay green and only production breaks. That's cheap to close at type level, and npm run test:typecheck already runs in CI:

const _none: powerbi.visuals.SubSelectionStylesType.None = SubSelectionStylesType.None; // one per member

I verified this catches drift on this branch — feeding a deliberately wrong value gives
error TS2322: Type '3' is not assignable to type 'SubSelectionStylesType.NumericText'.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

@@ -0,0 +1,4 @@
+import powerbiApiMock from "./mocks/powerbiApiMock";
+
+// Sources reference the ambient `powerbi` namespace for enum values, which does not exist at runtime.


This file is unused — please delete it along with the setupFiles entry in vitest.config.mts.

Verified both directions:

removing setupFiles and keeping the alias: 80 passed

removing the alias and keeping setupFiles: 3 failed suites, TypeError: Cannot read properties of undefined (reading 'SubSelectionOutlineRestrictionType')

So the powerbi-visuals-api alias is the load-bearing mechanism and this globalThis.powerbi shim never fires.

Alexey (Demonkratiy) · 2026-08-26T19:26:17Z

+            createSubSelectable(host, { objectName: "text", subSelectionType: SubSelectionStylesType.Text });
+            createSubSelectable(host, { objectName: "shape", subSelectionType: SubSelectionStylesType.Shape });
+
+            const subSelections = helper.getAllSubSelectables(SubSelectionStylesType.Shape)!;


Please add a SubSelectionStylesType.None case here — that's the one that exposes the if (filterType) guard in getAllSubSelectables (see my comment on src/HtmlSubSelectionHelper.ts). The existing case uses Shape (=3), which can't catch it.

Minor: the trailing ! is now redundant, since this PR narrowed the return type to CustomVisualSubSelection[]. Same at lines 510 and 548.

v-aidaba added 2 commits August 20, 2026 10:20

Add unit tests, modernize tooling and release 7.0.0 (ESM output)

43cf4d9

fix ci: typecheck and drop unused type-aware parsing from ESLint

2d33be2

Alexey (Demonkratiy) reviewed Aug 26, 2026

View reviewed changes

		@@ -0,0 +1,4 @@
		import powerbiApiMock from "./mocks/powerbiApiMock";

		// Sources reference the ambient `powerbi` namespace for enum values, which does not exist at runtime.

Uh oh!

Conversation

v-aidaba commented Aug 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Alexey (Demonkratiy) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

v-aidaba commented Aug 20, 2026 •

edited

Loading