Skip to content

Charts: stop pre-bundling @wordpress/ui so IconButton works in Script Module consumers #51016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adamwoodnz wants to merge 8 commits into
base: trunk
Choose a base branch
from
Open

Charts: stop pre-bundling @wordpress/ui so IconButton works in Script Module consumers #51016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
f837936
Charts: stop pre-bundling @wordpress/ui in the package build
adamwoodnz Aug 3, 2026
3261318
Charts: restore IconButton for the chart zoom-reset control
adamwoodnz Aug 3, 2026
db539b8
Charts: Remove redundant IconButton property overrides
adamwoodnz Aug 3, 2026
191a4e6
Charts: document the dist vs jetpack:src consumption split
adamwoodnz Aug 3, 2026
589e7ea
Charts: add changelog entry
adamwoodnz Aug 3, 2026
2dc08f9
Charts: record the runtime risk the build guard cannot catch
adamwoodnz Aug 4, 2026
47b6cd2
Improve comment
adamwoodnz Aug 5, 2026
4592a6a
Charts: mark the @wordpress/ui externalization as a breaking change
adamwoodnz Aug 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions projects/js-packages/charts/AGENTS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,36 @@ The package is migrating to WordPress UI and Theme as its defaults. When adding
`--wpds-*` mappings). Charts reference `--a8c-charts-*` roles with the mapped
`var(--wpds-*, <spec-fallback>)` as the inline fallback; there is no runtime
emission yet (that is CHARTS-203).
- **Two consumption paths — this changes what a charts change can break.**
`@wordpress/build` apps (premium-analytics, publicize, podcast, videopress)
consume the Rolldown output in `dist/` and load it as a **WordPress Script
Module** — native browser ESM, where `require` does not exist. Webpack apps
(My Jetpack and friends) resolve source through the `jetpack:src` export
condition instead. A change that only affects `dist/` can therefore break
four packages this one does not import.
Comment on lines +36 to +42

@manzoorwanijk manzoorwanijk Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hard-wrap makes it difficult to navigate or read for people which rely on screen readers.

- **Never `deps.alwaysBundle` a package that transitively requires an external.**
Pre-bundling is safe only for dependencies that require nothing themselves —
`fast-deep-equal` qualifies, which is why `tsdown.config.ts` still lists it.
Pre-bundle anything that reaches a CommonJS module requiring an external
(`react`, above all) and Rolldown emits a dynamic-`require` shim, because it
cannot rewrite a runtime `require` into a static ESM import. That shim throws
during module evaluation in Script Module consumers, taking down every widget
on the page rather than just the feature that pulled it in.
`tools/assert-no-dynamic-require.ts` fails the build when such a shim reaches
the ESM output; never suppress it.
- **`@wordpress/ui` is external in `dist`, and no build check can prove that is
safe.** Correctness depends on `@wordpress/build` *bundling* `@wordpress/ui`
rather than externalising it to `window.wp.ui`. It used to externalise it,
which is what CHARTS-163 worked around by pre-bundling; it now bundles any
`@wordpress/*` package that declares neither `wpScriptModuleExports` nor
`wpScript`, and `@wordpress/ui` declares `wpScript: false`. If a future
version reverts, `dist/index.js` keeps its clean `import … from
"@wordpress/ui"`, the guard passes, the build passes, and every Script Module
consumer breaks at runtime on `wp.ui` being undefined — the same blast radius
as CHARTS-237, with no build-time signal. Verified against `@wordpress/build`
0.18.0 (publicize, podcast, videopress) and 0.19.1-next (premium-analytics).
Check a major bump by loading a charts screen in wp-admin, not by trusting a
green build.

## Documentation Workflow

Expand Down
4 changes: 4 additions & 0 deletions projects/js-packages/charts/changelog/charts-247-externalize-wordpress-ui
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
Significance: major

@adamwoodnz adamwoodnz Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

major rather than minor, because this changes the published dist contract rather than an exported API.

Until now dist carried its own copy of @wordpress/ui. After this it imports it, so the host has to resolve it. That is fine in a plain bundler — it stays a dependency and resolves from node_modules — but it breaks any consumer that externalizes @wordpress/* to window.wp.*, because window.wp.ui does not exist.

That is not hypothetical. #47004 added the alwaysBundle this PR removes, precisely because externalized @wordpress/ui broke WooCommerce Analytics that way. Undoing it re-exposes the same surface, and the failure is a blank screen at runtime rather than a build error.

On the known consumer: the only woocommerce-analytics workspace that pairs @automattic/charts with @wordpress/build is next-woocommerce-analytics, and that one is abandoned — it became Jetpack Premium Analytics. Its other two workspaces are not exposed (standalone/ builds with @wordpress/build but has no charts dependency; the root has charts ^0.56.1, which cannot resolve to 1.x).

So in practice nothing live is likely to break. The major is not about today's consumer list though — charts ships to npm, so consumers are not enumerable, and minor would tell everyone on ^1.x this is safe to take unattended. Worth noting it makes the release 2.0.0 rather than 1.12.0.

Type: changed

Zoom: Restore the accessible tooltip on the reset control. `@wordpress/ui` is no longer bundled into the package output, so each consumer's bundler now resolves it. It remains a dependency and resolves from node_modules by default, but a bundler that externalizes `@wordpress/*` to `window.wp.*` must bundle `@wordpress/ui` instead — `window.wp.ui` does not exist.
2 changes: 1 addition & 1 deletion projects/js-packages/charts/src/charts/area-chart/stories/index.docs.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ Zoom behaviour:
- Only the X axis rescales; the Y axis is unaffected.
- A selection rectangle follows the pointer while dragging.
- Drags shorter than 6px are ignored, so a click never zooms.
- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space.
- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space. A "Reset zoom" tooltip shows on hover and on keyboard focus, and Escape dismisses it.
- Areas are clipped to the plot area whenever `zoomable` is set, keeping the zoom-out animation within the axes.
- `zoomable` chains with your own `onPointerDown`/`onPointerMove`/`onPointerUp` handlers rather than replacing them.

Expand Down
2 changes: 1 addition & 1 deletion projects/js-packages/charts/src/charts/line-chart/stories/index.docs.mdx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -527,7 +527,7 @@ Zoom behaviour:
- Only the X axis rescales; the Y axis is unaffected.
- A selection rectangle follows the pointer while dragging.
- Drags shorter than 6px are ignored, so a click never zooms.
- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain.
- While zoomed, a "Reset zoom" button appears in the top-right to restore the full domain. It is reachable with Tab and activates with Enter or Space. A "Reset zoom" tooltip shows on hover and on keyboard focus, and Escape dismisses it.
- Series are clipped to the plot area while zoomed, so lines never overflow the axes.
- `zoomable` chains with your own `onPointerDown`/`onPointerMove`/`onPointerUp` handlers rather than replacing them.

Expand Down
59 changes: 52 additions & 7 deletions projects/js-packages/charts/src/charts/private/x-zoom/test/x-zoom.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { act, render, renderHook, screen } from '@testing-library/react';
import { act, render, renderHook, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';
import { useRef } from 'react';
import { useCallback, useRef, useState } from 'react';
import { useXZoom, ZoomResetButton } from '../index';
import type { SingleChartRef } from '../../single-chart-context';
import type { EventHandlerParams } from '@visx/xychart';
Expand Down Expand Up @@ -147,17 +147,35 @@ const preventDefaultKeydown = ( event: ReactKeyboardEvent< HTMLDivElement > ) =>
event.preventDefault();

describe( 'ZoomResetButton', () => {
test( 'renders a labelled button with a hover tooltip', () => {
test( 'renders a labelled button without a native title tooltip', () => {
const noop = jest.fn();
render( <ZoomResetButton onClick={ noop } /> );
const button = screen.getByTestId( 'chart-zoom-reset' );
expect( button.tagName ).toBe( 'BUTTON' );
expect( button ).toHaveClass( 'x-zoom__reset' );
expect( button ).toHaveAccessibleName( 'Reset zoom' );
// WPDS `IconButton` would supply a tooltip of its own, but it pulls in a
// CommonJS dependency that breaks Script Module consumers (see
// ZoomResetButton). `title` restores the hover hint on plain `Button`.
expect( button ).toHaveAttribute( 'title', 'Reset zoom' );
// IconButton renders a real tooltip, so the `title` fallback is gone —
// `title` is invisible to keyboard users and cannot be dismissed.
expect( button ).not.toHaveAttribute( 'title' );
} );

test( 'shows a tooltip on keyboard focus', async () => {
const noop = jest.fn();
render( <ZoomResetButton onClick={ noop } /> );
await userEvent.tab();
expect( screen.getByTestId( 'chart-zoom-reset' ) ).toHaveFocus();
// The button's only text is the tooltip's — its own label is an
// `aria-label` attribute, so this cannot match the trigger.
await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible();
} );

test( 'dismisses the tooltip on Escape', async () => {
const noop = jest.fn();
render( <ZoomResetButton onClick={ noop } /> );
await userEvent.tab();
await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible();
await userEvent.keyboard( '{Escape}' );
await waitFor( () => expect( screen.queryByText( 'Reset zoom' ) ).not.toBeInTheDocument() );
} );

test( 'fires onClick when activated', async () => {
Expand All @@ -167,6 +185,33 @@ describe( 'ZoomResetButton', () => {
expect( onClick ).toHaveBeenCalledTimes( 1 );
} );

test( 'leaves no orphaned tooltip when activation unmounts the button', async () => {
// Resetting the zoom unmounts this control while its tooltip is open.
// The tooltip renders in a portal outside the container, so a missed
// cleanup would strand it on the page rather than remove it with the
// button.
/**
* Mirrors the host charts: the reset control exists only while zoomed.
*
* @return JSX element or null.
*/
function Host() {
const [ zoomed, setZoomed ] = useState( true );
const unzoom = useCallback( () => setZoomed( false ), [] );
return zoomed ? <ZoomResetButton onClick={ unzoom } /> : null;
}
render( <Host /> );
await userEvent.tab();
await expect( screen.findByText( 'Reset zoom' ) ).resolves.toBeVisible();

await userEvent.keyboard( '{Enter}' );

await waitFor( () =>
expect( screen.queryByTestId( 'chart-zoom-reset' ) ).not.toBeInTheDocument()
);
expect( document.body ).not.toHaveTextContent( 'Reset zoom' );
} );

test( 'keyboard activation survives the chart wrapper keydown handler', async () => {
const onClick = jest.fn();
// Mirrors the chart's grid wrapper, whose keyboard-navigation handler
Expand Down
22 changes: 6 additions & 16 deletions projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,29 +11,19 @@
pointer-events: none;
}

// Overlay placement and icon-only geometry — the visual treatment (border,
// hover/focus, sizing) comes from the WPDS Button. The elevation shadow is
// deliberately kept for separation from chart content (CHARTS-237
// design call, carrying over CHARTS-200's tokenization). `@wordpress/theme`
// 1.0.0 dropped the `--wpds-elevation-*` group, so this role carries the
// former token's spec value directly rather than nesting a `--wpds-*` var.
// The elevation shadow is deliberately kept for separation
// from chart content (CHARTS-237 design call, carrying
// over CHARTS-200's tokenization). `@wordpress/theme` 1.0.0
// dropped the `--wpds-elevation-*` group, so this role carries
// the former token's spec value directly rather than nesting
// a `--wpds-*` var.
&__reset {
position: absolute;
top: var(--wpds-dimension-gap-sm, 8px);
right: var(--wpds-dimension-gap-sm, 8px);
z-index: 2;
box-shadow: var(--a8c-charts-elevation-xs, 0 1px 1px 0 #00000008, 0 1px 2px 0 #00000005, 0 3px 3px 0 #00000005, 0 4px 4px 0 #00000003);

// `Button` is sized for a text label. These are the same three custom
// properties @wordpress/ui's own IconButton composition sets to make a
// square icon-only control; we set them directly because IconButton
// itself cannot be used here — it renders a Base UI tooltip, whose
// CommonJS `use-sync-external-store` dependency makes Rolldown emit a
// dynamic `require()` that throws in Script Module consumers.
--wp-ui-button-aspect-ratio: 1;
--wp-ui-button-padding-inline: 0;
--wp-ui-button-min-width: unset;

// WPDS outline-neutral is transparent-bodied at rest; give this
// floating control an opaque body so chart content doesn't show
// through it. Scoped to the rest state so the WPDS
Expand Down
69 changes: 29 additions & 40 deletions projects/js-packages/charts/src/charts/private/x-zoom/x-zoom.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { DataContext } from '@visx/xychart';
import { __ } from '@wordpress/i18n';
import { Button } from '@wordpress/ui';
import { IconButton } from '@wordpress/ui';
import { useCallback, useContext, useMemo, useState } from 'react';
import styles from './x-zoom.module.scss';
import type { SingleChartRef } from '../single-chart-context';
Expand Down Expand Up @@ -172,14 +172,8 @@ export function ZoomClip( {

/**
* Visible icon-only reset control rendered as an HTML overlay on top of the
* chart container, using the WPDS `Button`. The host should wrap its SVG in a
* `position: relative` container so the button anchors correctly.
*
* `IconButton` would be the natural fit, but it renders a Base UI tooltip whose
* CommonJS `use-sync-external-store` dependency makes Rolldown emit a dynamic
* `require()` into `dist`, which throws on evaluation in WordPress Script
* Module consumers. `Button` gives the same treatment without that dependency;
* the tooltip is replaced by `aria-label` + `title`.
* chart container, using the WPDS `IconButton`. The host should wrap its SVG in
* a `position: relative` container so the button anchors correctly.
*
* @param props - Props.
* @param props.onClick - Click handler. Typically the `reset` from `useXZoom`.
Expand All @@ -194,44 +188,39 @@ export function ZoomResetButton( { onClick }: { onClick: () => void } ) {
event.stopPropagation();
}
}, [] );
const label = __( 'Reset zoom', 'jetpack-charts' );
return (
<Button
<IconButton
className={ styles[ 'x-zoom__reset' ] }
onKeyDown={ stopActivationKeys }
aria-label={ label }
title={ label }
label={ __( 'Reset zoom', 'jetpack-charts' ) }
variant="outline"
tone="neutral"
size="small"
onClick={ onClick }
data-testid="chart-zoom-reset"
>
<Button.Icon
icon={
<svg
viewBox="0 0 24 24"
fill="none"
stroke="currentColor"
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
aria-hidden="true"
focusable="false"
>
{ /*
Icons render edge-to-edge at 24px, so inset the glyph the
way @wordpress/icons glyphs do (drawn within roughly 4-20
of the viewBox, ~1.5px effective stroke).
*/ }
<g transform="translate(2.4 2.4) scale(0.8)">
<circle cx="10" cy="10" r="6" />
<line x1="15" y1="15" x2="20" y2="20" />
<line x1="7" y1="10" x2="13" y2="10" />
</g>
</svg>
}
/>
</Button>
icon={
<svg
viewBox="0 0 24 24"
fill="none"
stroke="currentColor"
strokeWidth="2"
strokeLinecap="round"
strokeLinejoin="round"
aria-hidden="true"
focusable="false"
>
{ /*
Icons render edge-to-edge at 24px, so inset the glyph the
way @wordpress/icons glyphs do (drawn within roughly 4-20
of the viewBox, ~1.5px effective stroke).
*/ }
<g transform="translate(2.4 2.4) scale(0.8)">
<circle cx="10" cy="10" r="6" />
<line x1="15" y1="15" x2="20" y2="20" />
<line x1="7" y1="10" x2="13" y2="10" />
</g>
</svg>
}
/>
);
}
2 changes: 1 addition & 1 deletion projects/js-packages/charts/tsdown.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export default defineConfig( {
'.png': 'asset',
},
deps: {
alwaysBundle: [ '@wordpress/ui', /^fast-deep-equal/ ],

@manzoorwanijk manzoorwanijk Aug 5, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But wp build tools expect this to be bundled? How will this externalization deal with private APIs mismatch? Did we test this change with older versions of other plugins that consume the same package?

@adamwoodnz adamwoodnz Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good questions thanks 👍

"wp build tools expect this to be bundled?"

That was true, and it is exactly why CHARTS-163 / #47004 added alwaysBundle in the first place. It stopped being true: @wordpress/build now externalises only @wordpress/* packages that declare wpScriptModuleExports, and @wordpress/ui ships wpScript: false with neither, so wp-build bundles it itself.

packages/publicize already imports IconButton and Tooltip straight from @wordpress/ui and ships fine today, and publicize was one of the four packages the original incident broke.

"How will this externalization deal with private APIs mismatch?"

On instance duplication, this change avoids a mismatch rather than introducing one. @wordpress/ui depends on both @wordpress/private-apis and @wordpress/theme, and @wordpress/theme depends on @wordpress/private-apis too. Since alwaysBundle inlined @wordpress/ui but left @wordpress/theme external, anything that reaches private APIs through a bundled @wordpress/ui ends up registered in dist while pairing with a @wordpress/theme registered in the consumer's tree — two registries, one lock/unlock straddling them.

I built all three variants rather than reason about it:

dist/index.js size private-apis inlined @wordpress/* externals __require shims
trunk today (Button, ui bundled) 434,563 B 0 i18n, icons, theme 0
ui bundled + IconButton 709,228 B 2 i18n, icons, theme 5
this PR (ui external + IconButton) 350,627 B 0 i18n, icons, ui 0

Trunk today is fine, because Button never reaches the tooltip so the whole chain tree-shakes away. The middle row is the naive way to restore IconButton — keep alwaysBundle and swap the component — and that is where the mismatch appears: two private-apis registrations baked into dist, pairing with a @wordpress/theme registered in the consumer's tree. It is also where the five __require shims come from, so that row is the original incident.

Externalising avoids both: no private-apis in dist at all, one @wordpress/ui from the consumer bringing its own theme and private-apis from a single tree. Only one @wordpress/private-apis (1.51.0) resolves in the monorepo.

On version mismatch: Charts pins @wordpress/ui exactly (0.17.0, no range), so normal resolution keeps charts on that version no matter what a consumer installs — a mismatched consumer version installs alongside it rather than replacing it. I confirmed that by resolving from each package: charts and all five Script Module consumers get 0.17.0. The 0.18.0 copies in the tree arrive via the Gutenberg peer graph (block-editor, block-library, admin-ui, patterns) and never reach these packages.

So drift needs someone to override the pin — a resolutions / overrides entry or a forced bundler dedupe. That is a consumer choice rather than something this change imposes, though it is a fair thing to configure deliberately for a design system, precisely to avoid duplicate instances.

"Did we test this change with older versions of other plugins that consume the same package?"

Not initially — my first pass was all at monorepo HEAD, where every consumer builds from the same source, so that skew cannot occur by construction. I have since tested it.

The case I took you to mean is mixed vintages of the same package on one site: automattic/jetpack-publicize ships inside both plugins/jetpack and plugins/social, same for videopress and premium-analytics, so a site running Jetpack alongside a standalone plugin from a few releases back has two copies built against different charts.

I reproduced that deliberately. Built plugins/jetpack against trunk's charts (@wordpress/ui bundled) and plugins/social against this branch (external), leaving both plugins active on one site with a genuine vintage skew — confirmed by build timestamps, and by the old bundle still carrying the --wp-ui-button-* overrides the new one drops. Then loaded all three screens:

Screen Serving plugin charts vintage Result
Jetpack Social plugins/social new (ui external) loads, no errors
Podcasts plugins/jetpack old (ui bundled) loads, no errors
Premium Analytics plugins/premium-analytics new 33 chart elements, 0 widget errors

No critical errors, no Dynamic require of, and no private-apis unlock failures on any of them. That matches what I expected for the reason that each plugin's Script Module bundle is self-contained either way — previously charts' dist carried @wordpress/ui, now each consumer's wp-build bundles its own — so mixed vintages never share an instance.

What would break that is @wordpress/build externalising @wordpress/ui to a shared script-module handle, at which point two plugins registering the same handle at different versions really would collide. That is the failure mode I would watch on a wp-build major, and it is the same one the guard in this PR cannot detect.

That said, this is my reading of your question rather than necessarily yours. If you had a different combination in mind, tell me which and I will run that one instead.

@adamwoodnz adamwoodnz Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Update after rebasing onto trunk, since one detail above has moved underneath it.

Trunk bumped the @wordpress/ui pin to 0.19.0 while this was in review, so the 0.17.0 figure in the comment above is stale. The argument it supports is unchanged: the dependency is pinned exactly rather than by range, so a consumer cannot drift charts onto a different @wordpress/ui without an explicit resolutions / overrides entry or a forced dedupe, and charts and all five Script Module consumers still resolve the same version.

I re-measured the three build variants on 0.19.0 rather than assume the earlier figures carried. Same shape, slightly different numbers:

dist/index.js size private-apis inlined __require shims
trunk today (Button, ui bundled) 434,563 B 0 0
ui bundled + IconButton 708,251 B 2 5
this PR (ui external + IconButton) 351,644 B 0 0

Full suite passes on 0.19.0 — 1074 tests.

alwaysBundle: [ /^fast-deep-equal/ ],
},
css: {
fileName: 'index.css',
Expand Down
Loading