From 33da71c3d7e532e6966676fc400dc647a02135e7 Mon Sep 17 00:00:00 2001 From: chschan Date: Wed, 5 Aug 2026 21:39:44 +1000 Subject: [PATCH 1/7] Fix the four outstanding rhtmlBuildUtils defects, and port reviewBaselines Closes out the seven defects catalogued in the "rhtmlCombinedScatter CI will stop working from 30 Sep 2024" investigation. Three were already fixed on master, independently rather than from that investigation's branch: the Windows path corruption in compileRenderContentPage and the page.waitFor removal both came in with the esbuild work, and the jest ceiling was lifted by the jest 29 / jest-image-snapshot 6 / puppeteer 24 upgrade. The remaining four are here. Defect 3 -- a mismatching snapshot reported PASS. The catch around toMatchImageSnapshot swallowed the failure entirely, so the job only went red via jest's aggregate snapshotState.unmatched count and the per-test list said everything passed. Failures are now collected through the loop and rethrown once at the end: collected rather than thrown immediately because throwing inside the loop would abort it and lose the new_snapshots diagnostic image for every widget after the first. The diagnostic write also becomes synchronous; the old fs.writeFile callback form was fire-and-forget, so the process could exit before the image reached disk. This is the most important one of the four, because it makes the upcoming baseline review trustworthy. Defect 5 -- acceptNewSnapshots defaulted to true, so a snapshot with no baseline was written and PASSED. A newly added test could look green forever while never being regression-tested. Now defaults false. Defect 4 -- the --env whitelist allowed only 'local' and 'travis', so a CI environment could not be named after the system running it. rhtmlCombinedScatter had to bypass the flag entirely via snapshotTesting.env in its own config. Whitelist dropped. Defect 1 -- clean deleted man/, which holds TRACKED roxygen output that only makeDocs can regenerate, and makeDocs swallows its own failure so a missing R install is not fatal. `build` therefore silently deleted tracked R documentation on every machine without R on PATH, including all CI runners. man/ is out of the delete list. makeDocs also now runs `Rscript -e` rather than `r --no-save` with a bash herestring and POSIX redirects, none of which cmd.exe can parse -- so it could never have succeeded on Windows even with R installed. Also ports the reviewBaselines task from that investigation's branch, which existed nowhere else. It builds a local side-by-side review page because GitHub's diff renderer gives up on a few hundred binary files, which is exactly the size of a regenerated baseline set. It needed no conversion for the gulp removal -- it never took a gulp argument -- only its usage strings and the dropping of its `-t` alias for --to, since yargs is a shared singleton across task modules and -t already means testNamePattern in two other tasks. NB the investigation's branch cc-ci-fixes is NOT merged here and should not be. It branched before 8.0.0, so merging it would revert esbuild, the puppeteer and jest upgrade, jest.config.js and every .jest.test.js suite, and restore the old mocha processTestPlans.test.js. These fixes were re-applied onto current master instead. cc-ci-fixes can now be deleted. Two things found while doing this: * A real bug in the CLI added by the gulp removal: task names were taken as every argument not starting with '-', so a SPACE separated flag value was read as a task name. `rhtml reviewBaselines --from HEAD` failed with "unknown task 'HEAD'", and `rhtml testVisual -t someFilter` would have failed the same way -- which is the form rhtmlCombinedScatter's CI uses. Task names are now the leading positional arguments only, stopping at the first flag, which is what gulp did. Extracted to src/lib/parseTaskNames.js with tests, since it shipped silently once already. * A fidelity gap in the eslint config from the eslint 10 upgrade: @stylistic's customize() defaults operator-linebreak to 'before' but eslint-config-standard used 'after'. Nothing on master happened to wrap an operator, so the mismatch was invisible until a file written under the old config was added, which reported 13 errors for previously-correct style. Set to standard's value. Verified: eslint . clean, 84/84 jest tests pass across 9 suites, bin/prepush exits 0, and the fixture byte-diff from the gulp removal is still empty, so none of this changes build output. The six new tests for defect 3 were confirmed to FAIL against the old swallow-the-error behaviour (4 of 6 red) rather than merely passing against the new one. reviewBaselines was exercised end to end against a git fixture with a changed, an added, a removed and 18 identical baselines, plus both of its guard paths. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 28 +- eslint.config.base.js | 13 +- src/cli.js | 19 +- src/lib/parseTaskNames.jest.test.js | 44 +++ src/lib/parseTaskNames.js | 13 + .../renderExamplePageTest.helper.jest.test.js | 111 +++++++ src/lib/renderExamplePageTest.helper.js | 28 +- src/tasks/misc/clean.js | 24 +- src/tasks/misc/makeDocs.js | 20 +- src/tasks/snapshot/reviewBaselines/index.js | 278 ++++++++++++++++++ .../parseCommandLineArguments.js | 33 +++ .../parseCommandLineArguments.js | 18 +- 12 files changed, 597 insertions(+), 32 deletions(-) create mode 100644 src/lib/parseTaskNames.jest.test.js create mode 100644 src/lib/parseTaskNames.js create mode 100644 src/lib/renderExamplePageTest.helper.jest.test.js create mode 100644 src/tasks/snapshot/reviewBaselines/index.js create mode 100644 src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js diff --git a/README.md b/README.md index 2138f84..d487d9e 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,8 @@ automatically and are enumerated [below](#task-reference). ### Upgrading to 9.0.0 from 8.x -Two breaking changes, both requiring a small edit in the widget repo. +Two breaking changes requiring a small edit in the widget repo, plus three changes to what passes and +fails that need no edit but do change results. **1. gulp is gone.** Delete your `gulpfile.js`, drop `gulp` from your devDependencies, and change every npm script from `gulp ` to `rhtml `. Task names, sequences and command line flags are all @@ -79,6 +80,29 @@ byte for byte identical to what the gulp pipeline produced, which is deliberate, css feeds the pages the visual regression suite screenshots. `less` is pinned to 3.13.1 (the version `gulp-less@4` resolved) to keep it that way. +#### Three changes to what passes and fails + +These need no edit in your repo, but they will change your results, so sequence a bump deliberately. + +**A mismatching snapshot now fails its own test.** Previously the comparison error was swallowed, so a +test whose images did not match reported PASS and the job only went red via jest's aggregate count. +Reading the per-test list therefore led straight to the wrong conclusion. Expect previously-green runs +to surface real per-test failures. + +**A snapshot with no baseline now fails.** `acceptNewSnapshots` defaults to `false`. It used to default +to `true`, which appended `--ci=0` to the jest command and made jest write the missing baseline and pass +— so a newly added test could look green forever while never being regression-tested. Pass +`--acceptNewSnapshots` to opt back in when bootstrapping a suite. + +**`clean` no longer deletes `man/`.** That directory holds tracked roxygen output which only `makeDocs` +can regenerate, and `makeDocs` swallows its own failure so a missing R install is not fatal — so +`rhtml build` used to silently delete tracked R documentation on every machine without R on PATH, +including CI. `makeDocs` also now calls `Rscript` rather than `r`, which is what makes it capable of +succeeding on Windows at all. + +The `--env` flag also no longer has a `local`/`travis` whitelist, so a CI environment can be named after +the system running it instead of being set indirectly through `widget.config.js`. + Two of the main features provided by rhtmlBuildUtils are to start the internal web server and to run the visual regression tests. These topics are covered in these subdocs: * [internal web server](./docs/internal_web_server.md) @@ -151,6 +175,8 @@ The top level tasks are those you will likely run as part of the widget build pr `rhtml testVisual_s` : just run the visual regression suite (skip the other steps, `rhtml serve` must already be running). +`rhtml reviewBaselines --from [--to ]` : build a local side-by-side review page for image snapshot baselines, at `.tmp/reviewBaselines/index.html`. GitHub's diff renderer gives up on a few hundred binary files, which is exactly the size of a regenerated baseline set — and reviewing the images is the real gate when accepting new baselines, since a rendering regression accepted there is invisible afterwards. Omit `--to` to compare the working tree against ``. Baselines reported as *identical* are worth looking at first: one that did not regenerate usually means its test errored before reaching the snapshot. + `rhtml lint` : this runs the eslint style checker on all the javascript files. Our settings are defined in [eslint.config.base.js](./eslint.config.base.js), which your widget repo's `eslint.config.js` re-exports. Which files are checked is decided by the `ignores` in that config rather than by this task, because eslint 10 has no `.eslintignore`. To run with auto fix run `rhtml lint --fix`. Note that this is also run as a git prepush hook so you will not be able to push code to git unless it passes the style checks. # Developing / Contributing diff --git a/eslint.config.base.js b/eslint.config.base.js index 8c84414..0c927f5 100644 --- a/eslint.config.base.js +++ b/eslint.config.base.js @@ -109,13 +109,22 @@ module.exports = [ 'no-extend-native': 'error', 'no-new-func': 'error', - // @stylistic's customize() defaults are close to standard but differ on these five. Set to - // standard's values so no existing file is reformatted. + // @stylistic's customize() defaults are close to standard but differ on these. Set to standard's + // values so no existing file is reformatted. '@stylistic/comma-dangle': ['error', 'never'], '@stylistic/space-before-function-paren': ['error', 'always'], '@stylistic/brace-style': ['error', '1tbs', { allowSingleLine: true }], '@stylistic/quote-props': ['error', 'as-needed'], + // NB customize() defaults this to 'before', but eslint-config-standard used 'after' -- i.e. a + // wrapped expression keeps the operator at the END of the line. Nothing on master happened to + // wrap an operator, so the mismatch stayed invisible until a file written under the old config + // was added, which then reported 13 errors for style that was previously correct. Ternaries keep + // the operator at the start, which is standard's own exception. + '@stylistic/operator-linebreak': ['error', 'after', { + overrides: { '?': 'before', ':': 'before', '|>': 'before' } + }], + // Not enabled by eslint-config-standard, and the tree mixes `x => ...` with `(x) => ...`. // Turning it on would be a reformat, so leave the existing mix alone. '@stylistic/arrow-parens': 'off', diff --git a/src/cli.js b/src/cli.js index 664991d..9065f70 100644 --- a/src/cli.js +++ b/src/cli.js @@ -1,24 +1,21 @@ const colors = require('ansi-colors') -const { runTasks, knownTaskNames } = require('./index') +const { runTasks, knownTaskNames, widgetConfig } = require('./index') const { runTeardowns } = require('./lib/teardown') +const parseTaskNames = require('./lib/parseTaskNames') // Entry point for the `rhtml` binary. Replaces `gulp [...] [--flags]` with // `rhtml [...] [--flags]`. // -// NB task names are taken as the positional arguments and flags are left alone, because the tasks -// parse their own flags out of process.argv via yargs (--fix, --env, --branch, --port, -t, -u, ...). -// Deliberately not routed through a yargs command definition here: that would have to re-declare -// every flag of every task, and the task-local declarations are what the docs describe. -const parseTaskNames = (argv) => argv.filter(arg => !arg.startsWith('-')) - +// NB the flags themselves are deliberately NOT parsed here. Each task declares and reads its own flags +// out of process.argv via yargs (--fix, --env, --branch, --port, --from, -t, -u, ...). Routing them +// through a yargs command definition here would mean re-declaring every flag of every task, and the +// task-local declarations are what the docs describe. +// +// Which arguments are task names is decided by src/lib/parseTaskNames.js. const main = async () => { const requested = parseTaskNames(process.argv.slice(2)) const taskNames = requested.length ? requested : ['default'] - // NB required lazily: widgetConfig reads /build/config/widget.config.js at require - // time and throws if it is absent, and we want that to surface as a clear error after argv parsing - // rather than as a stack trace before the CLI has done anything. - const { widgetConfig } = require('./index') const disabledTasks = widgetConfig.disabledTasks || [] await runTasks({ taskNames, disabledTasks }) diff --git a/src/lib/parseTaskNames.jest.test.js b/src/lib/parseTaskNames.jest.test.js new file mode 100644 index 0000000..144a051 --- /dev/null +++ b/src/lib/parseTaskNames.jest.test.js @@ -0,0 +1,44 @@ +const parseTaskNames = require('./parseTaskNames') + +test('a single task', () => { + expect(parseTaskNames(['build'])).toEqual(['build']) +}) + +test('several tasks run in sequence', () => { + expect(parseTaskNames(['core', 'compileInternal'])).toEqual(['core', 'compileInternal']) +}) + +test('no arguments yields no tasks, so the caller can fall back to default', () => { + expect(parseTaskNames([])).toEqual([]) +}) + +test('an --opt=value flag is not a task', () => { + expect(parseTaskNames(['testVisual', '--env=local', '--branch=master'])).toEqual(['testVisual']) +}) + +// These are the regression. A space-separated flag value is a bare word, so filtering on "does not start +// with -" read it as a task name and failed with "unknown task 'HEAD'". Both forms below are used in +// anger: reviewBaselines takes `--from `, and rhtmlCombinedScatter's CI runs `-t "$TEST_FILTER"`. +test('a space separated --flag value is not a task', () => { + expect(parseTaskNames(['reviewBaselines', '--from', 'HEAD'])).toEqual(['reviewBaselines']) +}) + +test('a space separated short flag value is not a task', () => { + expect(parseTaskNames(['testVisual', '-t', 'someFilter'])).toEqual(['testVisual']) +}) + +test('several flag values after several tasks', () => { + expect(parseTaskNames(['reviewBaselines', '--from', 'abc123^', '--to', 'abc123'])) + .toEqual(['reviewBaselines']) +}) + +test('a boolean short flag with no value', () => { + expect(parseTaskNames(['testVisual_s', '-u'])).toEqual(['testVisual_s']) +}) + +// Flags before tasks are not supported: everything from the first flag on is flag territory, so the task +// name would be swallowed. Documented rather than fixed, because supporting it needs a per-flag arity +// table, which is exactly the re-declaration of every task's flags that the CLI avoids. +test('flags placed BEFORE the task names swallow them', () => { + expect(parseTaskNames(['--port', '9001', 'serve'])).toEqual([]) +}) diff --git a/src/lib/parseTaskNames.js b/src/lib/parseTaskNames.js new file mode 100644 index 0000000..8881eda --- /dev/null +++ b/src/lib/parseTaskNames.js @@ -0,0 +1,13 @@ +// Task names are the LEADING positional arguments of `rhtml ... [--flags]`, stopping at the first +// flag. Everything from the first flag onward belongs to the flags, which each task parses for itself. +// +// NB the stop-at-first-flag rule is the whole point. A space-separated flag VALUE is a bare word, so +// filtering on "does not start with -" reads the HEAD in `--from HEAD`, or the filter in +// `-t someFilter`, as a task name -- which fails with "unknown task 'HEAD'". gulp behaved the same way, +// taking task names first. +// +// Extracted from src/cli.js so it can be tested without executing the CLI, which calls process.exit. +module.exports = function parseTaskNames (argv) { + const firstFlag = argv.findIndex(arg => arg.startsWith('-')) + return (firstFlag === -1) ? argv : argv.slice(0, firstFlag) +} diff --git a/src/lib/renderExamplePageTest.helper.jest.test.js b/src/lib/renderExamplePageTest.helper.jest.test.js new file mode 100644 index 0000000..c507819 --- /dev/null +++ b/src/lib/renderExamplePageTest.helper.jest.test.js @@ -0,0 +1,111 @@ +const fs = require('fs-extra') +const os = require('os') +const path = require('path') + +// NB widgetConfig reads a build/config/widget.config from the consuming widget project, which does not +// exist in this repo, so it is stubbed. basePath is a real temp directory because these tests assert on +// the diagnostic png that testSnapshots writes when a comparison fails. +const mockBasePath = fs.mkdtempSync(path.join(os.tmpdir(), 'rbu-helper-')) + +jest.mock('./widgetConfig', () => ({ + basePath: mockBasePath, + snapshotTesting: { + snapshotDelay: 0, + snapshotDirectory: 'theSrc/test/snapshots', + env: 'local', + branch: 'master', + timeout: 1000, + pixelmatch: {} + }, + internalWebSettings: { + singleWidgetSnapshotSelector: '.widget', + statePreprocessor: x => x + } +})) + +const { testSnapshots } = require('./renderExamplePageTest.helper') + +const newSnapshotsDir = path.join(mockBasePath, 'theSrc/test/snapshots/local/master/new_snapshots') + +// A page whose $$ resolves `count` fake widgets, each screenshotting to a distinct buffer. +const fakePage = (count) => ({ + $$: async () => Array.from({ length: count }, (_unused, index) => ({ + screenshot: async () => Buffer.from(`image-${index}`) + })) +}) + +// Replaces the real jest-image-snapshot matcher so a comparison can be made to fail on demand without +// any actual image diffing. testSnapshots calls expect(image).toMatchImageSnapshot({...}). +const matcherFailingFor = (failingIdentifiers) => { + expect.extend({ + toMatchImageSnapshot (received, { customSnapshotIdentifier }) { + const pass = !failingIdentifiers.includes(customSnapshotIdentifier) + return { pass, message: () => `stub matcher: ${customSnapshotIdentifier} did not match` } + } + }) +} + +beforeEach(() => fs.removeSync(newSnapshotsDir)) +afterAll(() => fs.removeSync(mockBasePath)) + +test('resolves when every snapshot matches', async () => { + matcherFailingFor([]) + + await expect(testSnapshots({ page: fakePage(1), testName: 'all good' })).resolves.toBeUndefined() +}) + +// This is the regression the whole fix is for. The catch used to swallow the matcher's throw, so a test +// whose images did not match reported PASS -- the job only went red via jest's aggregate +// snapshotState.unmatched count, and reading the per-test list led to the wrong conclusion. +test('REJECTS when a snapshot does not match, rather than reporting pass', async () => { + matcherFailingFor(['a_mismatch']) + + await expect(testSnapshots({ page: fakePage(1), testName: 'a mismatch' })) + .rejects.toThrow(/1 snapshot\(s\) did not match: a_mismatch/) +}) + +test('names every failure, not just the first', async () => { + matcherFailingFor(['multi-one', 'multi-three']) + + const promise = testSnapshots({ + page: fakePage(3), + testName: 'multi', + snapshotNames: ['one', 'two', 'three'] + }) + + await expect(promise).rejects.toThrow(/2 snapshot\(s\) did not match: multi-one, multi-three/) +}) + +// Throwing from inside the loop would abort it, losing the diagnostic image for every widget after the +// first failure. Collecting and rethrowing at the end keeps the full set. +test('writes a diagnostic png for every failure, including ones after the first', async () => { + matcherFailingFor(['multi-one', 'multi-three']) + + await expect(testSnapshots({ + page: fakePage(3), + testName: 'multi', + snapshotNames: ['one', 'two', 'three'] + })).rejects.toThrow() + + expect(fs.readdirSync(newSnapshotsDir).sort()).toEqual(['multi-one-snap.png', 'multi-three-snap.png']) +}) + +// The previous implementation used the fs.writeFile callback form and never awaited it, so the process +// could exit before the diagnostic image reached disk. +test('the diagnostic png is on disk by the time the rejection surfaces', async () => { + matcherFailingFor(['sync_check']) + + await expect(testSnapshots({ page: fakePage(1), testName: 'sync check' })).rejects.toThrow() + + const written = path.join(newSnapshotsDir, 'sync_check-snap.png') + expect(fs.existsSync(written)).toBe(true) + expect(fs.readFileSync(written).toString()).toBe('image-0') +}) + +test('writes nothing to new_snapshots when everything matches', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(2), testName: 'clean run', snapshotNames: ['a', 'b'] }) + + expect(fs.existsSync(newSnapshotsDir)).toBe(false) +}) diff --git a/src/lib/renderExamplePageTest.helper.js b/src/lib/renderExamplePageTest.helper.js index 98b325a..31577cc 100644 --- a/src/lib/renderExamplePageTest.helper.js +++ b/src/lib/renderExamplePageTest.helper.js @@ -149,6 +149,18 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { } } + // NB failures are COLLECTED here and rethrown once at the end, rather than swallowed or thrown + // immediately. Two separate reasons: + // + // * Swallowing them (what this did before) meant a test whose images did not match reported PASS. + // The job only went red via jest's aggregate snapshotState.unmatched count, so reading the + // per-test list led straight to the wrong conclusion about what had actually failed. + // * Throwing from inside the loop would abort it, so a multi-widget test would lose the diagnostic + // new_snapshots image for every widget after the first failure. + // + // Collecting keeps the full set of diagnostics while still failing the test that failed. + const failures = [] + await asyncForEach(widgets, async (widget, index) => { // NB puppeteer now declares screenshot() as resolving a Uint8Array. It still hands back a // Buffer in practice, but jest-image-snapshot passes this straight to pngjs, which calls @@ -159,6 +171,8 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { try { expect(image).toMatchImageSnapshot({ customSnapshotIdentifier: snapshotName }) } catch (e) { + failures.push({ snapshotName, error: e }) + // Can't find group name so just put all new snapshots in same folder const snapshotDirectory = path.join( widgetConfig.basePath, @@ -167,12 +181,18 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { widgetConfig.snapshotTesting.branch ) const newSnapshotDir = path.join(snapshotDirectory, 'new_snapshots') - if (!fs.existsSync(newSnapshotDir)) fs.mkdirSync(newSnapshotDir) - fs.writeFile(path.join(newSnapshotDir, `${snapshotName}-snap.png`), image, 'binary', (err) => { - if (err) console.log('Error saving new image snapshot: ' + err) - }) + // NB synchronous: the previous fs.writeFile callback form was fire-and-forget, so the process + // could exit before the diagnostic image reached disk. + fs.mkdirpSync(newSnapshotDir) + fs.writeFileSync(path.join(newSnapshotDir, `${snapshotName}-snap.png`), image, 'binary') } }) + + if (failures.length) { + const names = failures.map(({ snapshotName }) => snapshotName).join(', ') + const detail = failures.map(({ error }) => error.message).join('\n\n') + throw new Error(`${failures.length} snapshot(s) did not match: ${names}\n\n${detail}`) + } } module.exports = { diff --git a/src/tasks/misc/clean.js b/src/tasks/misc/clean.js index 9a72e68..fd94e24 100644 --- a/src/tasks/misc/clean.js +++ b/src/tasks/misc/clean.js @@ -1,10 +1,22 @@ -const Promise = require('bluebird') -const fs = Promise.promisifyAll(require('fs-extra')) +const fs = require('fs-extra') +const path = require('path') +const { basePath } = require('../../lib/widgetConfig') + +// NB 'man' is deliberately NOT in this list. It holds roxygen output (man/*.Rd), which is TRACKED and +// which only the makeDocs task can regenerate -- and makeDocs shells out to R, then swallows its own +// failure so that a missing R install is not fatal. Deleting man/ here therefore destroyed tracked R +// documentation on every machine without R on PATH (every Windows dev, every CI runner) and left it +// destroyed, printing a "make docs failed" line and carrying on. +// +// The other entries are safe to delete because each is rebuilt by a step that cannot silently no-op: +// 'browser' and '.tmp' are build scratch, 'inst' comes from compileWidgetEntryPoint plus core, and 'R' +// from core's copy task. +const LOCATIONS_TO_DELETE = ['browser', 'inst', 'R', '.tmp'] module.exports = () => { - return function (done) { - const locationsToDelete = ['browser', 'inst', 'man', 'R', '.tmp'] - const deletePromises = locationsToDelete.map(function (location) { return fs.removeAsync(location) }) - Promise.all(deletePromises).then(function () { done() }) + return async function () { + // NB resolved against basePath rather than the process cwd, matching every other task. Under gulp + // these were cwd-relative and happened to work because gulp was always invoked from the widget root. + await Promise.all(LOCATIONS_TO_DELETE.map(location => fs.remove(path.join(basePath, location)))) } } diff --git a/src/tasks/misc/makeDocs.js b/src/tasks/misc/makeDocs.js index 197c37b..5b4a8c4 100644 --- a/src/tasks/misc/makeDocs.js +++ b/src/tasks/misc/makeDocs.js @@ -1,10 +1,22 @@ const shell = require('shelljs') +// NB Rscript, not `r`. littler (`r`) is not available on Windows and is uncommon elsewhere, whereas +// Rscript ships with every R install. The old command also used a bash herestring (<<<) and POSIX +// output redirects, neither of which cmd.exe can parse, so on Windows it could never succeed even with +// R installed. shelljs's own `silent` option replaces the redirects. +const COMMAND = 'Rscript -e "library(devtools); document()"' + module.exports = () => { return function (done) { - const commandString = 'r --no-save 2>/dev/null >/dev/null <<< "library(devtools); document()"' - const exitCode = shell.exec(commandString).code - if (exitCode !== 0) console.log(`make docs failed with code ${exitCode}. Command was '${commandString}'`) - done(null) // don't trigger failure (e.g. if R is not installed) + const exitCode = shell.exec(COMMAND, { silent: true }).code + + // Still non-fatal: R and devtools are genuinely optional for JS-only development, and that is why + // this task swallows its exit code. What changed is that `clean` no longer deletes man/, so a + // skipped makeDocs now leaves the committed documentation intact rather than leaving a hole. + if (exitCode !== 0) { + console.log(`make docs skipped: '${COMMAND}' exited ${exitCode}. ` + + 'This is not fatal -- R and devtools are optional. man/*.Rd is left as committed.') + } + done(null) } } diff --git a/src/tasks/snapshot/reviewBaselines/index.js b/src/tasks/snapshot/reviewBaselines/index.js new file mode 100644 index 0000000..8040a25 --- /dev/null +++ b/src/tasks/snapshot/reviewBaselines/index.js @@ -0,0 +1,278 @@ +// Builds a local side-by-side review page for image snapshot baselines. +// +// Why this exists: GitHub's diff renderer gives up on a few hundred binary +// files ("Unable to render code block"), which is exactly the size of a +// regenerated baseline set. Reviewing the images is the real gate when accepting +// new baselines -- a rendering regression accepted there is invisible afterwards +// -- so it needs to be possible outside the PR view. +// +// Usage: +// rhtml reviewBaselines --from HEAD compare working tree against HEAD +// rhtml reviewBaselines --from abc123^ --to abc123 compare a commit against its parent +// +// Output: .tmp/reviewBaselines/index.html + +const _ = require('lodash') +const fs = require('fs-extra') +const path = require('path') +const shell = require('shelljs') +const widgetConfig = require('../../../lib/widgetConfig') +const getCommandLineArgs = require('./parseCommandLineArguments') + +const WORKING_TREE = '(working tree)' + +module.exports = () => { + return function (done) { + const args = getCommandLineArgs() + + if (!args.from) { + return done(new Error( + 'reviewBaselines needs --from , the baselines to compare against.\n' + + ' rhtml reviewBaselines --from HEAD\n' + + ' rhtml reviewBaselines --from abc123^ --to abc123' + )) + } + + const { snapshotTesting, basePath } = widgetConfig + const env = args.env || snapshotTesting.env + const branch = args.branch || snapshotTesting.branch + const snapshotPath = [snapshotTesting.snapshotDirectory, env, branch].join('/') + + const outputDirectory = path.join(basePath, '.tmp', 'reviewBaselines') + fs.removeSync(outputDirectory) + fs.mkdirpSync(outputDirectory) + + // Guard against comparing a ref with itself, which would report every + // baseline as identical -- a silently wrong answer from a tool whose whole + // job is spotting differences. + if (args.to) { + const resolve = (ref) => shell.exec(`git rev-parse "${ref}"`, { cwd: basePath, silent: true }) + const fromSha = resolve(args.from) + const toSha = resolve(args.to) + if (fromSha.code === 0 && toSha.code === 0 && fromSha.stdout.trim() === toSha.stdout.trim()) { + return done(new Error( + `--from "${args.from}" and --to "${args.to}" both resolve to ${fromSha.stdout.trim().slice(0, 7)}, ` + + 'so there is nothing to compare.' + )) + } + } + + let from, to + try { + from = materialise({ ref: args.from, snapshotPath, outputDirectory, basePath, name: 'from' }) + to = args.to + ? materialise({ ref: args.to, snapshotPath, outputDirectory, basePath, name: 'to' }) + : { + root: path.join(basePath, ...snapshotPath.split('/')), + label: WORKING_TREE, + // index.html sits at /.tmp/reviewBaselines/, so the working + // tree is two levels up + relativeToOutput: `../../${snapshotPath}` + } + } catch (error) { + return done(error) + } + + const pairs = collectPairs({ from, to }) + if (!pairs.length) { + return done(new Error( + `No .png baselines found under ${snapshotPath} in either ref. ` + + 'Check --env and --branch: the path is //.' + )) + } + + const outputFile = path.join(outputDirectory, 'index.html') + fs.writeFileSync(outputFile, renderPage({ pairs, from, to, snapshotPath }), 'utf8') + + const counts = _.countBy(pairs, 'status') + console.log(`reviewBaselines: ${pairs.length} baseline(s) under ${snapshotPath}`) + console.log(` ${_.map(counts, (n, status) => `${n} ${status}`).join(', ')}`) + console.log(` open ${outputFile}`) + done() + } +} + +// Extract the snapshot tree at a ref into the output directory. Deliberately +// avoids `git archive | tar` -- the pipe is unreliable under shelljs on Windows, +// which shells out through cmd.exe. Writing the tar then extracting it works on +// both, since Windows has shipped bsdtar since 1803. +const materialise = ({ ref, snapshotPath, outputDirectory, basePath, name }) => { + const destination = path.join(outputDirectory, name) + const tarFile = path.join(outputDirectory, `${name}.tar`) + fs.mkdirpSync(destination) + + // The ref is quoted because shelljs shells through cmd.exe on Windows, where + // ^ is the escape character -- an unquoted "HEAD^" silently becomes "HEAD", + // which would compare a commit against itself and report no differences. + const archive = shell.exec( + `git archive --format=tar --output="${tarFile}" "${ref}" -- "${snapshotPath}"`, + { cwd: basePath, silent: true } + ) + if (archive.code !== 0) { + throw new Error(`Could not read ${snapshotPath} at "${ref}": ${archive.stderr.trim()}`) + } + + // Extract with cwd set to the destination and a RELATIVE path to the archive. + // GNU tar (which is what Git Bash provides on Windows) reads an absolute + // Windows path as an rsh-style host spec and fails with + // "Cannot connect to C: resolve failed". Relative paths avoid the drive letter + // and work under both GNU tar and the bsdtar that ships with Windows. + const extract = shell.exec(`tar -xf "../${path.basename(tarFile)}"`, { + cwd: destination, silent: true + }) + if (extract.code !== 0) { + throw new Error(`Could not extract the snapshot archive for "${ref}": ${extract.stderr.trim()}`) + } + fs.removeSync(tarFile) + + return { + root: path.join(destination, ...snapshotPath.split('/')), + label: ref, + relativeToOutput: `${name}/${snapshotPath}` + } +} + +const findPngs = (root) => { + if (!fs.existsSync(root)) { return [] } + const walk = (directory) => _.flatMap(fs.readdirSync(directory, { withFileTypes: true }), (entry) => { + const full = path.join(directory, entry.name) + if (entry.isDirectory()) { return walk(full) } + return entry.name.endsWith('.png') ? [path.relative(root, full).split(path.sep).join('/')] : [] + }) + return walk(root) +} + +const collectPairs = ({ from, to }) => { + const relativePaths = _.union(findPngs(from.root), findPngs(to.root)).sort() + + return relativePaths.map((relativePath) => { + const fromFile = path.join(from.root, ...relativePath.split('/')) + const toFile = path.join(to.root, ...relativePath.split('/')) + const hasFrom = fs.existsSync(fromFile) + const hasTo = fs.existsSync(toFile) + + let status + if (!hasFrom) { status = 'added' } else if (!hasTo) { status = 'removed' } else { + status = fs.readFileSync(fromFile).equals(fs.readFileSync(toFile)) ? 'identical' : 'changed' + } + + return { + relativePath, + status, + collection: relativePath.includes('/') ? relativePath.replace(/\/[^/]+$/, '') : '(root)', + name: relativePath.replace(/^.*\//, ''), + fromSrc: hasFrom ? `${from.relativeToOutput}/${relativePath}` : null, + toSrc: hasTo ? `${to.relativeToOutput}/${relativePath}` : null + } + }) +} + +const escapeHtml = (value) => String(value) + .replace(/&/g, '&').replace(//g, '>').replace(/"/g, '"') + +const LABELS = { + changed: 'changed', + added: 'new - no baseline in --from', + removed: 'removed', + identical: 'identical - not regenerated' +} + +const renderPage = ({ pairs, from, to, snapshotPath }) => { + const counts = _.countBy(pairs, 'status') + const summary = _(['changed', 'added', 'removed', 'identical']) + .filter((status) => counts[status]) + .map((status) => `${counts[status]} ${status}`) + .join(' · ') + + const body = _(pairs) + .groupBy('collection') + .map((items, collection) => { + const heading = `

${escapeHtml(collection)} ` + + `${_.map(_.countBy(items, 'status'), (n, s) => `${n} ${s}`).join(' ')}

` + return heading + items.map(renderPair).join('\n') + }) + .join('\n') + + return ` + +Baseline review: ${escapeHtml(from.label)} to ${escapeHtml(to.label)} + +

Baseline review — ${escapeHtml(snapshotPath)}

+

Left: ${escapeHtml(from.label)}. Right: ${escapeHtml(to.label)}. +Accepting these images makes them the definition of correct, so a regression accepted here is invisible afterwards. +Start with identical: a baseline that did not regenerate usually means its test errored before reaching the snapshot.

+
+ ${summary} + + + + + + +
+${body} + +` +} + +const renderPair = ({ relativePath, status, name, fromSrc, toSrc }) => { + const side = (src, caption, missingText) => '
' + caption + '
' + + (src + ? `${escapeHtml(relativePath)}` + : `
${missingText}
`) + '
' + + return `
` + + `
${escapeHtml(name)}` + + `${LABELS[status]}
` + + '
' + + side(fromSrc, 'before', 'no baseline') + + side(toSrc, 'after', 'removed') + + '
' +} diff --git a/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js b/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js new file mode 100644 index 0000000..fcb878f --- /dev/null +++ b/src/tasks/snapshot/reviewBaselines/parseCommandLineArguments.js @@ -0,0 +1,33 @@ +// NB avoid defaults for env and branch: if a default is specified then the arg +// object contains the value regardless of whether it was given on the command +// line, which would override the widget's own config. Same convention as +// takeSnapshotsForEachTestDefinition. + +const yargs = require('yargs') + +module.exports = () => { + yargs.option('from', { + alias: 'f', + string: true, + describe: 'git ref holding the baselines to compare against, e.g. HEAD or a commit sha' + }) + // NB deliberately no short alias. `-t` would be the obvious one, but yargs is a shared singleton + // across the task modules and -t already means testNamePattern in jestSpecTests and + // takeSnapshotsForEachTestDefinition. Declaring it twice with different meanings is a trap for + // whoever hits it. + yargs.option('to', { + string: true, + describe: 'git ref holding the new baselines. Omit to use the working tree' + }) + yargs.option('env', { + alias: 'e', + string: true, + describe: 'which snapshot env to review (defaults to the widget config)' + }) + yargs.option('branch', { + alias: 'b', + string: true, + describe: 'which snapshot branch to review (defaults to the widget config)' + }) + return yargs.parse() +} diff --git a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js index 35d0d28..7000605 100644 --- a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js +++ b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/parseCommandLineArguments.js @@ -5,22 +5,32 @@ const yargs = require('yargs') module.exports = () => { + // NB defaults to FALSE so that a snapshot with no baseline FAILS instead of being written and + // passed. When true, this appends --ci=0 to the jest command, which makes jest compute + // updateSnapshot: 'new' (jest-config/build/normalize.js), so jest-image-snapshot writes the missing + // baseline and the test goes green. Combined with baselines only leaving CI on an explicit + // regeneration, a newly added test could look green forever while never being regression-tested at + // all. Pass --acceptNewSnapshots to opt back in when bootstrapping a new suite. yargs.option('acceptNewSnapshots', { alias: 'a', - describe: 'accept new snapshots', + describe: 'write and pass snapshots that have no baseline, instead of failing', boolean: true, - default: true + default: false }) yargs.option('branch', { alias: 'b', string: true, describe: 'which branch are we testing (used to choose snapshot set)' }) + // NB no `choices` whitelist. It previously allowed only 'local' and 'travis', so a CI environment + // could not be named after the system actually running it -- rhtmlCombinedScatter had to bypass the + // flag entirely by setting snapshotTesting.env in its own widget.config.js, which works only because + // options without defaults are absent from the parsed args. The value is just a directory name under + // snapshotDirectory, so any string is valid. Travis has not been in use for years. yargs.option('env', { alias: 'e', string: true, - describe: 'which env are we testing (used to choose snapshot set)', - choices: ['local', 'travis'] + describe: 'which env are we testing, e.g. local or ci (chooses the snapshot set)' }) yargs.option('headless', { alias: 'h', From 8b84259928af33150b73b60f7ca2da7fea90838c Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 09:53:23 +1000 Subject: [PATCH 2/7] Teach the shared eslint config that widget source is browser ESM The base config declared CommonJS globally, via eslint-plugin-n's flat/recommended-script. That is correct for this package's own source and for most of a widget repo, but WRONG for the one part that matters most to a widget: theSrc/scripts, the standardised home of the widget's own code, which is browser ES modules bundled by esbuild. Found while migrating rhtmlCombinedScatter: adopting the shared config gave a "'import' and 'export' may appear only with sourceType: module" parsing error on every one of its 36 source files. It parsed under the old .eslintrc only because eslint-config-standard set sourceType: 'module' for the whole project. So the existing browser-ESM override now also covers theSrc/scripts/**/*.js and theSrc/internal_www/js/**/*.js, which brings browser globals and the disabling of the node-oriented n/* rules with it. Globs that do not apply in a given repo match nothing, so one list serves both this package and its consumers. Verified: eslint . clean and 84/84 tests still pass here, and the config now parses rhtmlCombinedScatter's source. The ESM boundary there is exactly theSrc/scripts -- 36 files, nothing else in the repo uses import/export. Co-Authored-By: Claude Opus 5 (1M context) --- eslint.config.base.js | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/eslint.config.base.js b/eslint.config.base.js index 0c927f5..24ec6b9 100644 --- a/eslint.config.base.js +++ b/eslint.config.base.js @@ -49,11 +49,24 @@ const browserContextFiles = [ // the test run for the same reason. const copiedTemplateTests = ['src/tasks/*/assets/*.jest.test.js'] -// The experiment UI is browser ES modules (`import _ from 'lodash'`), bundled by esbuild. It parsed -// under the old .eslintrc only because eslint-config-standard set sourceType: 'module' for the whole -// project, including the CommonJS majority. eslint-plugin-n's recommended-script config is correctly -// CommonJS, so the genuinely-ESM files now say so for themselves. -const browserEsmFiles = ['src/tasks/experiment/assets/ui/**/*.js'] +// Browser ES modules (`import _ from 'lodash'`), bundled by esbuild -- as opposed to the node CommonJS +// that everything else here and in a widget repo is written in. These parsed under the old .eslintrc +// only because eslint-config-standard set sourceType: 'module' for the WHOLE project, including the +// CommonJS majority; eslint-plugin-n's recommended-script config is correctly CommonJS, so the +// genuinely-ESM files have to say so for themselves. +// +// NB the widget entries matter as much as this package's own. `theSrc/scripts` is the standardised home +// for a widget's source -- both widgetEntryPoint and widgetFactory point into it (see +// src/config/default.widget.config.js) -- and it is ALL browser ESM. Without these globs a widget repo +// adopting this config gets a parsing error on every one of its own source files. Globs that do not +// apply in a given repo simply match nothing, so one list serves both. +const browserEsmFiles = [ + // this package's own experiment UI + 'src/tasks/experiment/assets/ui/**/*.js', + // a widget repo's source, and any browser javascript it serves from the internal web server + 'theSrc/scripts/**/*.js', + 'theSrc/internal_www/js/**/*.js' +] module.exports = [ { ignores }, From 681b28c51a66245761af2814843f544b31b6c091 Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 09:59:28 +1000 Subject: [PATCH 3/7] Alias node's buffer builtin for esbuild, alongside crypto The alias map shimmed `crypto` to crypto-browserify because browserify used to provide node builtins implicitly and esbuild does not. That shim was incomplete: crypto-browserify's own dependency tree -- asn1.js, browserify-sign, browserify-rsa, safe-buffer -- requires 'buffer' in 19 places, so aliasing crypto without also aliasing buffer fails the bundle outright with "Could not resolve buffer". Found while migrating rhtmlCombinedScatter, whose dependency graph reaches crypto and which therefore could not build at all against 9.0.0: `rhtml core compileWidgetEntryPoint` exited 1 with 19 unresolved-buffer errors. Verified: eslint . clean and 84/84 tests still pass here. Co-Authored-By: Claude Opus 5 (1M context) --- package-lock.json | 68 +++++++++++++++++++++++++++++++++++++++++++ package.json | 1 + src/lib/compileES6.js | 12 +++++++- 3 files changed, 80 insertions(+), 1 deletion(-) diff --git a/package-lock.json b/package-lock.json index 58a9a2e..9cdd5f0 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16,6 +16,7 @@ "app-root-dir": "^1.0.2", "babel-jest": "^29.7.0", "bluebird": "^3.7.2", + "buffer": "^6.0.3", "chokidar": "^4.0.3", "connect": "^3.7.0", "connect-livereload": "^0.6.1", @@ -48,6 +49,9 @@ "shelljs": "^0.8.4", "yargs": "^15.4.1" }, + "bin": { + "rhtml": "bin/rhtml.js" + }, "engines": { "node": "^20.19.0 || ^22.13.0 || >=24" } @@ -3585,6 +3589,26 @@ "bare-path": "^3.0.0" } }, + "node_modules/base64-js": { + "version": "1.5.1", + "resolved": "https://registry.npmjs.org/base64-js/-/base64-js-1.5.1.tgz", + "integrity": "sha512-AKpaYlHn8t4SVbOHCy+b5+KKgvR4vrsD8vbvrbiQJps7fKDTkjkDry6ji0rUJjC0kzbNePLwzxq8iypo41qeWA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT" + }, "node_modules/baseline-browser-mapping": { "version": "2.11.8", "resolved": "https://registry.npmjs.org/baseline-browser-mapping/-/baseline-browser-mapping-2.11.8.tgz", @@ -3777,6 +3801,30 @@ "node-int64": "^0.4.0" } }, + "node_modules/buffer": { + "version": "6.0.3", + "resolved": "https://registry.npmjs.org/buffer/-/buffer-6.0.3.tgz", + "integrity": "sha512-FTiCpNxtwiZZHEZbcbTIcZjERVICn9yq/pDFkTl95/AxzD1naBctN7YO68riM/gLSDY7sdrMby8hofADYuuqOA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "MIT", + "dependencies": { + "base64-js": "^1.3.1", + "ieee754": "^1.2.1" + } + }, "node_modules/buffer-crc32": { "version": "0.2.13", "resolved": "https://registry.npmjs.org/buffer-crc32/-/buffer-crc32-0.2.13.tgz", @@ -5934,6 +5982,26 @@ "node": ">=0.8.0" } }, + "node_modules/ieee754": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/ieee754/-/ieee754-1.2.1.tgz", + "integrity": "sha512-dcyqhDvX1C46lXZcVqCpK+FtMRQVdIMN6/Df5js2zouUsqG7I6sFxitIC+7KYK29KdXOLHdu9zL4sFnoVQnqaA==", + "funding": [ + { + "type": "github", + "url": "https://github.com/sponsors/feross" + }, + { + "type": "patreon", + "url": "https://www.patreon.com/feross" + }, + { + "type": "consulting", + "url": "https://feross.org/support" + } + ], + "license": "BSD-3-Clause" + }, "node_modules/ignore": { "version": "5.3.2", "resolved": "https://registry.npmjs.org/ignore/-/ignore-5.3.2.tgz", diff --git a/package.json b/package.json index 5506d1d..3fb874d 100644 --- a/package.json +++ b/package.json @@ -25,6 +25,7 @@ "app-root-dir": "^1.0.2", "babel-jest": "^29.7.0", "bluebird": "^3.7.2", + "buffer": "^6.0.3", "chokidar": "^4.0.3", "connect": "^3.7.0", "connect-livereload": "^0.6.1", diff --git a/src/lib/compileES6.js b/src/lib/compileES6.js index d9db489..f6d02c0 100644 --- a/src/lib/compileES6.js +++ b/src/lib/compileES6.js @@ -31,7 +31,17 @@ module.exports = ({ entryPointFile, destinationDirectory, minify = false, callba format: 'iife', logLevel: 'silent', // we surface errors and warnings ourselves, below inject: [path.join(__dirname, 'esbuildPolyfillShim.js')], - alias: { crypto: 'crypto-browserify' }, // browserify shimmed node builtins implicitly + // browserify shimmed node builtins implicitly; esbuild does not, so the ones actually reached by + // widget dependency graphs are mapped here. + // + // NB `buffer` is not optional alongside `crypto`. crypto-browserify's own dependency tree + // (asn1.js, browserify-sign, browserify-rsa, safe-buffer, ...) requires 'buffer' in 19 places, so + // aliasing crypto without it fails the bundle outright with "Could not resolve buffer". Found while + // migrating rhtmlCombinedScatter, whose graph reaches crypto and which therefore could not build. + alias: { + crypto: 'crypto-browserify', + buffer: 'buffer' + }, define: { 'process.env.NODE_ENV': JSON.stringify(minify ? 'production' : 'development'), global: 'window' From 3067a83d2926b95a355fe6c9d741ed7d3c618c03 Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 10:01:30 +1000 Subject: [PATCH 4/7] Complete the node builtin shim set for esbuild: buffer, stream, events The alias map shimmed `crypto` to crypto-browserify because browserify used to provide node builtins implicitly and esbuild does not. That shim was incomplete. crypto-browserify's own dependency tree -- asn1.js, browserify-sign, browserify-rsa, safe-buffer, cipher-base, hash-base, readable-stream -- requires buffer, stream and events, so aliasing crypto alone fails the bundle outright, first with 19 "Could not resolve buffer" errors and then with 4 more for stream and events. Found while migrating rhtmlCombinedScatter, which could not build at all against 9.0.0: `rhtml core compileWidgetEntryPoint` exited 1. Its chain is bignumber.js@2.4.0, whose minified build requires crypto for BigNumber.random, then the tree above. Any widget depending on bignumber.js v2 hits exactly this, which is why it belongs in the shared alias map rather than in one widget's esbuildOptions escape hatch. Verified: eslint clean here, and the rhtmlCombinedScatter bundle now builds. Co-Authored-By: Claude Opus 5 (1M context) --- package-lock.json | 35 +++++++++++++++++++++++++++++++++++ package.json | 2 ++ src/lib/compileES6.js | 16 +++++++++++----- 3 files changed, 48 insertions(+), 5 deletions(-) diff --git a/package-lock.json b/package-lock.json index 9cdd5f0..9721787 100644 --- a/package-lock.json +++ b/package-lock.json @@ -26,6 +26,7 @@ "eslint": "^10.8.0", "eslint-plugin-n": "^18.2.2", "eslint-plugin-promise": "^7.3.0", + "events": "^3.3.0", "fancy-log": "^1.3.3", "fast-glob": "^3.3.3", "fs-extra": "^9.1.0", @@ -47,6 +48,7 @@ "serve-index": "^1.9.1", "serve-static": "^1.14.1", "shelljs": "^0.8.4", + "stream-browserify": "^3.0.0", "yargs": "^15.4.1" }, "bin": { @@ -5201,6 +5203,15 @@ "node": ">= 0.6" } }, + "node_modules/events": { + "version": "3.3.0", + "resolved": "https://registry.npmjs.org/events/-/events-3.3.0.tgz", + "integrity": "sha512-mQw+2fkQbALzQ7V0MY0IqdnXNOeTtP4r0lN9z7AAawCXgqea7bDii20AYrIBrFd/Hx0M2Ocz6S111CaFkUcb0Q==", + "license": "MIT", + "engines": { + "node": ">=0.8.x" + } + }, "node_modules/events-universal": { "version": "1.0.1", "resolved": "https://registry.npmjs.org/events-universal/-/events-universal-1.0.1.tgz", @@ -8950,6 +8961,30 @@ "node": ">= 0.6" } }, + "node_modules/stream-browserify": { + "version": "3.0.0", + "resolved": "https://registry.npmjs.org/stream-browserify/-/stream-browserify-3.0.0.tgz", + "integrity": "sha512-H73RAHsVBapbim0tU2JwwOiXUj+fikfiaoYAKHF3VJfA0pe2BCzkhAHBlLG6REzE+2WNZcxOXjK7lkso+9euLA==", + "license": "MIT", + "dependencies": { + "inherits": "~2.0.4", + "readable-stream": "^3.5.0" + } + }, + "node_modules/stream-browserify/node_modules/readable-stream": { + "version": "3.6.2", + "resolved": "https://registry.npmjs.org/readable-stream/-/readable-stream-3.6.2.tgz", + "integrity": "sha512-9u/sniCrY3D5WdsERHzHE4G2YCXqoG5FTHUiCC4SIbr6XcLZBY05ya9EKjYek9O5xOAwjGq+1JdGBAS7Q9ScoA==", + "license": "MIT", + "dependencies": { + "inherits": "^2.0.3", + "string_decoder": "^1.1.1", + "util-deprecate": "^1.0.1" + }, + "engines": { + "node": ">= 6" + } + }, "node_modules/streamx": { "version": "2.28.0", "resolved": "https://registry.npmjs.org/streamx/-/streamx-2.28.0.tgz", diff --git a/package.json b/package.json index 3fb874d..2514743 100644 --- a/package.json +++ b/package.json @@ -35,6 +35,7 @@ "eslint": "^10.8.0", "eslint-plugin-n": "^18.2.2", "eslint-plugin-promise": "^7.3.0", + "events": "^3.3.0", "fancy-log": "^1.3.3", "fast-glob": "^3.3.3", "fs-extra": "^9.1.0", @@ -56,6 +57,7 @@ "serve-index": "^1.9.1", "serve-static": "^1.14.1", "shelljs": "^0.8.4", + "stream-browserify": "^3.0.0", "yargs": "^15.4.1" }, "overrides": { diff --git a/src/lib/compileES6.js b/src/lib/compileES6.js index f6d02c0..ae6d20b 100644 --- a/src/lib/compileES6.js +++ b/src/lib/compileES6.js @@ -34,13 +34,19 @@ module.exports = ({ entryPointFile, destinationDirectory, minify = false, callba // browserify shimmed node builtins implicitly; esbuild does not, so the ones actually reached by // widget dependency graphs are mapped here. // - // NB `buffer` is not optional alongside `crypto`. crypto-browserify's own dependency tree - // (asn1.js, browserify-sign, browserify-rsa, safe-buffer, ...) requires 'buffer' in 19 places, so - // aliasing crypto without it fails the bundle outright with "Could not resolve buffer". Found while - // migrating rhtmlCombinedScatter, whose graph reaches crypto and which therefore could not build. + // NB these four go together. Aliasing `crypto` alone is not enough: crypto-browserify's own + // dependency tree (asn1.js, browserify-sign, browserify-rsa, safe-buffer, cipher-base, hash-base, + // readable-stream) requires buffer, stream and events, so a crypto-only alias fails the bundle + // outright with "Could not resolve buffer" / "Could not resolve stream". + // + // Found while migrating rhtmlCombinedScatter, which could not build at all against 9.0.0. Its chain + // is bignumber.js@2 -> crypto (used for BigNumber.random) -> the tree above. Any widget depending on + // bignumber.js v2 hits exactly this, so it belongs here rather than in one widget's esbuildOptions. alias: { crypto: 'crypto-browserify', - buffer: 'buffer' + buffer: 'buffer', + stream: 'stream-browserify', + events: 'events' }, define: { 'process.env.NODE_ENV': JSON.stringify(minify ? 'production' : 'development'), From 53523151cfb86f47020c2e819b3b8c729261c0a3 Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 11:12:46 +1000 Subject: [PATCH 5/7] Stub crypto by default instead of mapping it to crypto-browserify The esbuild alias map inherited `crypto: 'crypto-browserify'` from the browserify->esbuild migration, on the reasoning that browserify shimmed node builtins implicitly. For crypto that reasoning does not hold, because browserify never actually shipped it for these widgets. bignumber.js@2 -- rhtmlCombinedScatter and rhtmlLabeledScatter both depend on it -- reaches for crypto with if ( !cryptoObj ) try { cryptoObj = require('cry' + 'pto'); } catch (e) {} The concatenation and the try/catch are a deliberate bundler-evasion idiom: they stop static resolution, and the catch makes runtime absence harmless. browserify's scanner only matches literal string arguments, so it saw no dependency and emitted no crypto. esbuild constant-folds the concatenation, so it DOES resolve, and the alias then pulled 616 KiB across 180 files (elliptic, four separate copies of bn.js, asn1.js, browserify-sign, diffie-hellman) into rhtmlCombinedScatter's bundle -- 1651 KiB to 2341 KiB -- for BigNumber.random, which nothing calls. So the default is now src/lib/cryptoStub.js, which reproduces what browserify shipped. A widget that genuinely needs crypto opts back in from its own config: esbuildOptions: { alias: { crypto: 'crypto-browserify' } } NB rhtmlPictographs IS such a widget: CacheService.js and SvgDefinitionManager.js call crypto.createHash for cache keys. It is pinned to 7.2.3 so nothing breaks today, but it must add the opt-in when it migrates. Checked the other ten dependent repos; none references crypto in its own source. Documented in the README upgrade notes, naming Pictographs. The stub's members throw rather than being absent, so that case fails with a message naming the fix rather than "crypto.createHash is not a function". Everything else is left undefined, which is what bignumber's own feature detection expects. buffer/stream/events stay aliased even though the stub no longer needs them: they are what makes the crypto-browserify opt-in resolve at all, they are the right answer for widget code that requires them directly, and they cost nothing when nothing does. Verified: eslint clean, 84/84 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 18 ++++++++++++++++++ src/lib/compileES6.js | 22 ++++++++++++++-------- src/lib/cryptoStub.js | 42 ++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 74 insertions(+), 8 deletions(-) create mode 100644 src/lib/cryptoStub.js diff --git a/README.md b/README.md index d487d9e..c52b218 100644 --- a/README.md +++ b/README.md @@ -103,6 +103,24 @@ succeeding on Windows at all. The `--env` flag also no longer has a `local`/`travis` whitelist, so a CI environment can be named after the system running it instead of being set indirectly through `widget.config.js`. +#### `crypto` is stubbed in the bundle + +**If your widget calls anything on node's `crypto`, you must opt back in.** Add to +`build/config/widget.config.js`: + + esbuildOptions: { alias: { crypto: 'crypto-browserify' } } + +Without it the first call throws with a message pointing back here, rather than failing silently. +**rhtmlPictographs is the known case** — `CacheService.js` and `SvgDefinitionManager.js` both use +`crypto.createHash`. + +Why the default changed: `bignumber.js@2` (rhtmlCombinedScatter, rhtmlLabeledScatter) reaches for crypto +via `require('cry' + 'pto')` inside a `try/catch`, an idiom specifically intended to stop bundlers +resolving it — and browserify duly shipped none of it. esbuild constant-folds the concatenation, so it +resolves, and mapping it to `crypto-browserify` dragged 616 KiB across 180 files (`elliptic`, four copies +of `bn.js`, `asn1.js`, `diffie-hellman`) into rhtmlCombinedScatter's bundle for a code path +(`BigNumber.random`) that nothing calls. See [src/lib/cryptoStub.js](./src/lib/cryptoStub.js). + Two of the main features provided by rhtmlBuildUtils are to start the internal web server and to run the visual regression tests. These topics are covered in these subdocs: * [internal web server](./docs/internal_web_server.md) diff --git a/src/lib/compileES6.js b/src/lib/compileES6.js index ae6d20b..a573c6b 100644 --- a/src/lib/compileES6.js +++ b/src/lib/compileES6.js @@ -34,16 +34,22 @@ module.exports = ({ entryPointFile, destinationDirectory, minify = false, callba // browserify shimmed node builtins implicitly; esbuild does not, so the ones actually reached by // widget dependency graphs are mapped here. // - // NB these four go together. Aliasing `crypto` alone is not enough: crypto-browserify's own - // dependency tree (asn1.js, browserify-sign, browserify-rsa, safe-buffer, cipher-base, hash-base, - // readable-stream) requires buffer, stream and events, so a crypto-only alias fails the bundle - // outright with "Could not resolve buffer" / "Could not resolve stream". + // browserify shimmed node builtins implicitly; esbuild does not, so the ones widget dependency + // graphs actually reach are mapped here. // - // Found while migrating rhtmlCombinedScatter, which could not build at all against 9.0.0. Its chain - // is bignumber.js@2 -> crypto (used for BigNumber.random) -> the tree above. Any widget depending on - // bignumber.js v2 hits exactly this, so it belongs here rather than in one widget's esbuildOptions. + // NB `crypto` is stubbed, NOT mapped to crypto-browserify. See src/lib/cryptoStub.js for the full + // reasoning: browserify shipped no crypto at all for these widgets, and resolving it costs 616 KiB + // for a code path nothing calls. A widget that genuinely needs it opts back in via + // esbuildOptions: { alias: { crypto: 'crypto-browserify' } }. + // + // NB buffer/stream/events stay mapped even though the default crypto stub no longer needs them. + // They are what makes the crypto-browserify opt-in above actually resolve -- its tree (asn1.js, + // browserify-sign, safe-buffer, cipher-base, hash-base, readable-stream) requires all three, and + // without them the bundle fails with "Could not resolve buffer" / "Could not resolve stream". They + // are also the right answer for any widget code that requires them directly, and cost nothing when + // nothing does. alias: { - crypto: 'crypto-browserify', + crypto: path.join(__dirname, 'cryptoStub.js'), buffer: 'buffer', stream: 'stream-browserify', events: 'events' diff --git a/src/lib/cryptoStub.js b/src/lib/cryptoStub.js new file mode 100644 index 0000000..500b197 --- /dev/null +++ b/src/lib/cryptoStub.js @@ -0,0 +1,42 @@ +// Stands in for node's `crypto` when esbuild bundles a widget. Aliased in src/lib/compileES6.js. +// +// Why a stub is the DEFAULT rather than crypto-browserify: +// +// bignumber.js@2 -- used by rhtmlCombinedScatter and rhtmlLabeledScatter -- reaches for crypto with +// +// if ( !cryptoObj ) try { cryptoObj = require('cry' + 'pto'); } catch (e) {} +// +// The string concatenation and the try/catch are deliberate: they stop a bundler statically resolving +// crypto, and browserify duly shipped none of it. esbuild constant-folds the concatenation, so it DOES +// resolve, and aliasing to crypto-browserify then pulled 616 KiB across 180 files (elliptic, four separate +// copies of bn.js, asn1.js, browserify-sign, diffie-hellman) into rhtmlCombinedScatter's bundle, taking it +// from 1651 to 2341 KiB -- all for a code path (BigNumber.random) that nothing calls. Stubbing reproduces +// what browserify shipped for years. +// +// A widget that genuinely needs crypto in the browser opts back in from its widget.config.js: +// +// esbuildOptions: { alias: { crypto: 'crypto-browserify' } } +// +// NB rhtmlPictographs is the known case: CacheService.js and SvgDefinitionManager.js call +// crypto.createHash for cache keys, so it must opt in when it moves off 7.2.3. +// +// NB the members below THROW rather than being absent, so a widget that needs crypto fails with an +// actionable message rather than "crypto.createHash is not a function". Everything else is undefined, +// which is what bignumber's own feature detection expects. +const unavailable = (name) => () => { + throw new Error( + `node's crypto.${name}() is not in this bundle. rhtmlBuildUtils stubs 'crypto' by default, because ` + + 'resolving it pulls ~600 KiB of crypto-browserify into the widget for a code path that is usually ' + + 'never called. If this widget really does need crypto in the browser, opt back in from ' + + 'build/config/widget.config.js with: esbuildOptions: { alias: { crypto: \'crypto-browserify\' } }' + ) +} + +module.exports = { + createHash: unavailable('createHash'), + createHmac: unavailable('createHmac'), + randomBytes: unavailable('randomBytes'), + randomFillSync: unavailable('randomFillSync'), + pbkdf2: unavailable('pbkdf2'), + pbkdf2Sync: unavailable('pbkdf2Sync') +} From b55978593877c552935280570549074669b79af3 Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 11:57:21 +1000 Subject: [PATCH 6/7] Restore the -snap suffix on baseline filenames jest-image-snapshot changed how it names baseline files between v3 and v6. Its createSnapshotIdentifier now reads let snapshotIdentifier = customSnapshotIdentifier || `${defaultIdentifier}-snap` so '-snap' is appended ONLY when no custom identifier is supplied, and the baseline is written as `${snapshotIdentifier}.png`. v3 appended it either way, which is why all 877 committed baselines in rhtmlCombinedScatter -- and every other widget repo -- are named -snap.png. testSnapshots passes a custom identifier, so under v6 it was producing .png. Nothing matched the existing baselines, every test looked new, and the first regeneration run on rhtmlCombinedScatter wrote a parallel un-suffixed set while orphaning the entire committed one. The suffix is now applied at the call site. Added two tests that pin the identifier, single- and multi-widget. The stub matcher now records what it was asked for. This failure mode is silent -- the suite goes green, the file names are simply wrong -- so it needs a test rather than a comment. Verified: 86/86 tests pass, eslint clean. Co-Authored-By: Claude Opus 5 (1M context) --- .../renderExamplePageTest.helper.jest.test.js | 38 ++++++++++++++++--- src/lib/renderExamplePageTest.helper.js | 12 +++++- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/src/lib/renderExamplePageTest.helper.jest.test.js b/src/lib/renderExamplePageTest.helper.jest.test.js index c507819..659f608 100644 --- a/src/lib/renderExamplePageTest.helper.jest.test.js +++ b/src/lib/renderExamplePageTest.helper.jest.test.js @@ -35,10 +35,15 @@ const fakePage = (count) => ({ }) // Replaces the real jest-image-snapshot matcher so a comparison can be made to fail on demand without -// any actual image diffing. testSnapshots calls expect(image).toMatchImageSnapshot({...}). +// any actual image diffing, and so the identifier testSnapshots asks for can be asserted on. +// testSnapshots calls expect(image).toMatchImageSnapshot({...}). +let requestedIdentifiers = [] + const matcherFailingFor = (failingIdentifiers) => { + requestedIdentifiers = [] expect.extend({ toMatchImageSnapshot (received, { customSnapshotIdentifier }) { + requestedIdentifiers.push(customSnapshotIdentifier) const pass = !failingIdentifiers.includes(customSnapshotIdentifier) return { pass, message: () => `stub matcher: ${customSnapshotIdentifier} did not match` } } @@ -58,14 +63,14 @@ test('resolves when every snapshot matches', async () => { // whose images did not match reported PASS -- the job only went red via jest's aggregate // snapshotState.unmatched count, and reading the per-test list led to the wrong conclusion. test('REJECTS when a snapshot does not match, rather than reporting pass', async () => { - matcherFailingFor(['a_mismatch']) + matcherFailingFor(['a_mismatch-snap']) await expect(testSnapshots({ page: fakePage(1), testName: 'a mismatch' })) .rejects.toThrow(/1 snapshot\(s\) did not match: a_mismatch/) }) test('names every failure, not just the first', async () => { - matcherFailingFor(['multi-one', 'multi-three']) + matcherFailingFor(['multi-one-snap', 'multi-three-snap']) const promise = testSnapshots({ page: fakePage(3), @@ -79,7 +84,7 @@ test('names every failure, not just the first', async () => { // Throwing from inside the loop would abort it, losing the diagnostic image for every widget after the // first failure. Collecting and rethrowing at the end keeps the full set. test('writes a diagnostic png for every failure, including ones after the first', async () => { - matcherFailingFor(['multi-one', 'multi-three']) + matcherFailingFor(['multi-one-snap', 'multi-three-snap']) await expect(testSnapshots({ page: fakePage(3), @@ -93,7 +98,7 @@ test('writes a diagnostic png for every failure, including ones after the first' // The previous implementation used the fs.writeFile callback form and never awaited it, so the process // could exit before the diagnostic image reached disk. test('the diagnostic png is on disk by the time the rejection surfaces', async () => { - matcherFailingFor(['sync_check']) + matcherFailingFor(['sync_check-snap']) await expect(testSnapshots({ page: fakePage(1), testName: 'sync check' })).rejects.toThrow() @@ -109,3 +114,26 @@ test('writes nothing to new_snapshots when everything matches', async () => { expect(fs.existsSync(newSnapshotsDir)).toBe(false) }) + +// These two pin the baseline FILENAME, which is the contract with every committed baseline in every +// widget repo: they are all -snap.png. jest-image-snapshot v3 appended '-snap' to a custom +// identifier; v6 appends it only when no custom identifier is given +// (`customSnapshotIdentifier || `${defaultIdentifier}-snap``) and writes `${identifier}.png`. So under +// v6 the suffix has to come from here. Getting this wrong is silent and expensive: nothing matches the +// existing baselines, every test looks new, and a regeneration writes a parallel un-suffixed set while +// orphaning the old one -- which is exactly what happened on the first rhtmlCombinedScatter run. +test('asks for an identifier ending in -snap, so the baseline is -snap.png', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(1), testName: 'grid' }) + + expect(requestedIdentifiers).toEqual(['grid-snap']) +}) + +test('suffixes every widget on a multi-widget page', async () => { + matcherFailingFor([]) + + await testSnapshots({ page: fakePage(3), testName: 'multi', snapshotNames: ['one', 'two', 'three'] }) + + expect(requestedIdentifiers).toEqual(['multi-one-snap', 'multi-two-snap', 'multi-three-snap']) +}) diff --git a/src/lib/renderExamplePageTest.helper.js b/src/lib/renderExamplePageTest.helper.js index 31577cc..ce3d39f 100644 --- a/src/lib/renderExamplePageTest.helper.js +++ b/src/lib/renderExamplePageTest.helper.js @@ -169,7 +169,17 @@ const testSnapshots = async ({ page, testName, snapshotNames = null }) => { let image = Buffer.isBuffer(rawImage) ? rawImage : Buffer.from(rawImage) const snapshotName = getSnapshotName(index) try { - expect(image).toMatchImageSnapshot({ customSnapshotIdentifier: snapshotName }) + // NB the '-snap' suffix is applied HERE, and must be. jest-image-snapshot changed this between + // v3 and v6. Its createSnapshotIdentifier now reads: + // + // let snapshotIdentifier = customSnapshotIdentifier || `${defaultIdentifier}-snap` + // + // so '-snap' is appended only when NO custom identifier is given -- and the baseline file is + // `${snapshotIdentifier}.png`. v3 appended it either way, which is why every committed baseline + // in every widget repo is named -snap.png. Passing the bare name under v6 writes + // .png instead: nothing matches the existing baselines, every test looks new, and a + // regeneration silently produces a parallel un-suffixed set while orphaning all the old ones. + expect(image).toMatchImageSnapshot({ customSnapshotIdentifier: `${snapshotName}-snap` }) } catch (e) { failures.push({ snapshotName, error: e }) From 0a00449f077a32eb30128cacf39cb85fd1bd61f3 Mon Sep 17 00:00:00 2001 From: chschan Date: Thu, 6 Aug 2026 16:43:22 +1000 Subject: [PATCH 7/7] Delete the snapshot pass-through config once the jest run finishes .tmp/snapshot_dynamic_config.json exists only to carry command line values across a process boundary: takeSnapshotsForEachTestDefinition shells out to jest, and jest propagates no arguments to the workers that read widgetConfig. But widgetConfig merges that file at HIGHER precedence than the widget's own build/config/widget.config.js, and nothing ever removed it -- so it became ambient state that silently reconfigured every LATER task. Found in rhtmlCombinedScatter. A filtered probe run, rhtml testVisual --env=local --branch=probe --snapshotDirectory=.tmp/probe2 left `rhtml reviewBaselines` reading .tmp/probe2/local/probe instead of the real snapshot tree, and it failed with "Could not read .tmp/probe2/local/probe at HEAD". That is exactly the moment reviewBaselines has to be right: straight after a test run, when you are about to accept 430 regenerated baselines. Now removed in the shell.exec callback, with force: true so a run that never got as far as writing it does not fail on the way out. NB takeExperimentSnapshots writes its own separate pass-through file, which widgetConfig does not read, so it does not leak this way. Verified: eslint clean, 86/86 tests pass. Co-Authored-By: Claude Opus 5 (1M context) --- .../index.js | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js index 80c8479..cff14e3 100644 --- a/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js +++ b/src/tasks/snapshot/takeSnapshotsForEachTestDefinition/index.js @@ -39,6 +39,7 @@ module.exports = () => { // connect now registers its server with src/lib/teardown.js, so the runner closes it and owns the // exit code, and this task is free to appear anywhere in a sequence. return shell.exec(command, { async: true }, (exitCode) => { + removePassThroughConfigFile({ widgetConfig }) const error = (exitCode === 0) ? null : new Error(`${command} failed with code ${exitCode}`) done(error) }) @@ -67,6 +68,11 @@ const getCommandString = ({ testRoots, jestPath, args }) => { return `"${jestPath}" ${roots} ${testFilePattern} ${acceptNewSnapshots} ${updateSnapshots} ${testNamePattern}` } +// The only reason this file exists is to carry command line values across a process boundary: this task +// shells out to jest, and jest propagates no arguments to the workers that actually read widgetConfig. +const passThroughConfigPath = ({ widgetConfig }) => + path.join(widgetConfig.basePath, '.tmp', 'snapshot_dynamic_config.json') + const writePassThroughConfigFile = ({ widgetConfig, args }) => { const dynamicSnapshotConfig = _.pick(args, ['branch', 'env', 'snapshotDirectory']) @@ -79,6 +85,20 @@ const writePassThroughConfigFile = ({ widgetConfig, args }) => { } const configString = JSON.stringify(dynamicSnapshotConfig, {}, 2) - fs.writeFileSync(path.join(widgetConfig.basePath, '.tmp', 'snapshot_dynamic_config.json'), configString, 'utf8') + fs.writeFileSync(passThroughConfigPath({ widgetConfig }), configString, 'utf8') if (ECHO_PASSTHROUGH_CONFIG) { console.log(`snapshot dynamic config: ${configString}`) } } + +// NB removed as soon as the jest run finishes, because widgetConfig merges this file at HIGHER precedence +// than the widget's own build/config/widget.config.js. Left behind, it silently reconfigures every LATER +// task that reads widgetConfig -- so a filtered run like +// +// rhtml testVisual --env=local --branch=probe --snapshotDirectory=.tmp/probe +// +// left `rhtml reviewBaselines` pointing at .tmp/probe rather than the real snapshot tree, which is exactly +// when you need reviewBaselines to be right. It is a process-boundary hack, not ambient state, so it +// should not outlive the process it was written for. +const removePassThroughConfigFile = ({ widgetConfig }) => { + // force: true so a run that never got as far as writing the file does not fail on the way out. + fs.rmSync(passThroughConfigPath({ widgetConfig }), { force: true }) +}