Fix the four outstanding CI-investigation defects, and port reviewBaselines

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 <task>` to `rhtml <task>`. Task names, sequences and command line flags are all
@@ -79,6 +80,47 @@ 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`.
+
+#### `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)
@@ -151,6 +193,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 <ref> [--to <ref>]` : 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 `<ref>`. 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

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 },
@@ -109,13 +122,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',

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",
@@ -34,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",
@@ -55,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": {

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 <task> [<task>...] [--flags]` with
 // `rhtml <task> [<task>...] [--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 <widget repo>/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 })

src/lib/compileES6.js

@@ -31,7 +31,29 @@ 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.
+    //
+    // browserify shimmed node builtins implicitly; esbuild does not, so the ones widget dependency
+    // graphs actually reach are mapped here.
+    //
+    // 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: path.join(__dirname, 'cryptoStub.js'),
+      buffer: 'buffer',
+      stream: 'stream-browserify',
+      events: 'events'
+    },
     define: {
       'process.env.NODE_ENV': JSON.stringify(minify ? 'production' : 'development'),
       global: 'window'