Conversation
fastfrwrd
requested review from
Copilot and
s4kh
and removed request for
Copilot
fastfrwrd
added
patch
|
I assume I will have to import them differently for this to actually work, but I'm going to see how the canary does against Grafana now. |
There was a problem hiding this comment.
Pull request overview
This PR makes the axe-core accessibility testing peer dependencies optional to avoid forcing plugin authors to install dependencies they may not need. The changes convert regular imports to type-only imports and use dynamic imports with error handling for runtime usage.
Changes:
- Added
peerDependenciesMetato mark@axe-core/playwrightandaxe-coreas optional peer dependencies - Converted static imports to type-only imports using
import typesyntax - Implemented dynamic import with try-catch for runtime usage in the fixture
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/plugin-e2e/package.json | Added peerDependenciesMeta to mark axe-core dependencies as optional |
| packages/plugin-e2e/src/matchers/toHaveNoA11yViolations.ts | Converted imports to type-only imports for optional dependencies |
| packages/plugin-e2e/src/fixtures/scanForA11yViolations.ts | Added dynamic import with error handling for optional dependency |
| return accessibilityScanResults; | ||
| }); | ||
| } catch (error) { | ||
| throw new Error('@axe-core/playwright must be installed as a peer dependency to use a11y scanning.'); |
There was a problem hiding this comment.
The error handling is incorrect for this fixture. When the dynamic import fails, the catch block throws an error without calling use(), which will cause the fixture setup itself to fail. This means tests that declare the fixture in their parameters will fail even if they never use it.
Instead, the fixture should call use() with a function that throws the error when invoked. This way, only tests that actually call scanForA11yViolations() will see the error, while tests that merely have the fixture available (but don't use it) will still pass.
| throw new Error('@axe-core/playwright must be installed as a peer dependency to use a11y scanning.'); | |
| await use(async () => { | |
| throw new Error('@axe-core/playwright must be installed as a peer dependency to use a11y scanning.'); | |
| }); |
There was a problem hiding this comment.
point taken; wrote it a different way.
grafana-plugins-platform-bot
bot
moved this from 📬 Triage
to 🔬 In review
in Grafana Catalog Team
Playwright test results
Troubleshooting404 when clicking on
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (2)
packages/plugin-e2e/src/fixtures/scanForA11yViolations.ts:16
- Even with
peerDependenciesMeta.optional, importingAxeResultsfromaxe-corein this exported fixture type means TypeScript consumers may still needaxe-coreinstalled just to typecheck (because the generated.d.tswill referenceaxe-core). If the goal is truly “optional unless you use a11y”, consider replacingAxeResultsin the public signature with an internal minimal result type (orunknown/structural type) so projects that don’t use a11y don’t need the axe packages for TS resolution.
import { TestFixture } from '@playwright/test';
import type { AxeResults } from 'axe-core';
import type { AxeBuilder as AxeBuilderType } from '@axe-core/playwright';
import { PlaywrightArgs, AxeScanContext } from '../types';
export const DEFAULT_A11Y_TAGS = ['wcag2a', 'wcag2aa', 'wcag21a', 'wcag21aa' /* 'best-practice' */];
let AxeBuilder: typeof AxeBuilderType;
/**
* @alpha - the API for accessibility scanning is still being finalized and may change in future releases. Feedback is welcome!
*/
export const scanForA11yViolations: TestFixture<
(context?: AxeScanContext) => Promise<AxeResults>,
PlaywrightArgs
packages/plugin-e2e/src/matchers/toHaveNoA11yViolations.ts:13
AxeResultsis pulled fromaxe-core(now an optional peer), but this matcher’s public API still references that external type. For TS consumers who don’t install the optional peer deps, this can still break typechecking because the emitted.d.tswill referenceaxe-core. To make the dependency truly optional, consider changing the matcher signature/docs to use a local structural “axe results” type (only the fields you access:violations[].id/description/helpUrl/impact/nodes[].html) instead ofaxe-core’s exported type.
import type { MatcherReturnType } from '@playwright/test';
import type { AxeResults } from 'axe-core';
import { A11yViolationsOptions } from '../types';
/**
* @alpha - the API is not yet stable and may change without a major version bump. Use with caution.
* @param {AxeResults} results - The results from an Axe accessibility scan.
* @param {A11yViolationsOptions} options - Options for configuring the accessibility violations check.
* @returns {MatcherReturnType} - The result of the accessibility violations check.
*/
export function toHaveNoA11yViolations(results: AxeResults, options?: A11yViolationsOptions): MatcherReturnType {
const threshold = options?.threshold ?? 0;
| try { | ||
| const { AxeBuilder: _AxeBuilder } = await import('@axe-core/playwright'); | ||
| AxeBuilder = _AxeBuilder; |
There was a problem hiding this comment.
The dynamic import is pulling a named export ({ AxeBuilder }), but this module was previously imported as the default export (import AxeBuilder from '@axe-core/playwright'). If @axe-core/playwright only provides a default export, _AxeBuilder will be undefined and new AxeBuilder({ page }) will crash. Align the dynamic import (and the AxeBuilderType typing) with the package’s actual export shape (likely default).
| const { AxeBuilder: _AxeBuilder } = await import('@axe-core/playwright'); | ||
| AxeBuilder = _AxeBuilder; | ||
| } catch (error) { | ||
| throw new Error('@axe-core/playwright must be installed as a peer dependency to use a11y scanning.'); |
There was a problem hiding this comment.
The thrown message says only @axe-core/playwright must be installed, but axe-core is also declared as an (optional) peer and may be the actual missing module that makes this import fail. Update the error to mention both packages (or make it generic like “axe a11y peer deps”) so users know what to install.
| throw new Error('@axe-core/playwright must be installed as a peer dependency to use a11y scanning.'); | |
| throw new Error('@axe-core/playwright and axe-core must be installed (as peer dependencies) to use a11y scanning.'); |
There was a problem hiding this comment.
that optional dependency only impacts TypeScript types, which seem to not be an issue unless we forget to import type, so we don't care I think.
|
I tested this via the canary on grafana/grafana and the Polystat plugin - this change does effectively make the peer dependency optional. When you run an a11y test in Polystat, you get the more useful error, and when you install the Axe dep, the test passes. final version of the branch is here
|
I'm a little concerned about the peerDependencies in 3.4.0 after integrating in Grafana, in case some plugin authors don't want to use the a11y tests, so I'm going to see if they can be made optional in an elegant way. We should maybe release this as 3.4.1 in case 3.4.0 is a problem for folks.
📦 Published PR as canary version:
Canary Versions✨ Test out this PR locally via:
npm install @grafana/plugin-e2e@3.4.1-canary.2485.22416356262.0 # or yarn add @grafana/plugin-e2e@3.4.1-canary.2485.22416356262.0