From 5d77bd50496dd377b2999c49720f595025414ee2 Mon Sep 17 00:00:00 2001 From: naorpeled Date: Sun, 30 Aug 2026 22:38:01 +0300 Subject: [PATCH] fix: getBrandVersion throws on ES5 browsers; make test:types runnable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from Qodo review on #632. `Parser.getBrandVersion()` called `Array.prototype.find` directly. That is ES6, and `es5.js` ships no polyfills, so a documented public API threw `TypeError: this._hints.brands.find is not a function` on exactly the browsers that bundle exists to serve. `bundled.js` was unaffected because core-js polyfills it. Every other lookup in parser.js already goes through `Utils.find`, which guards on `Array.prototype.find` and falls back to a loop; this one call site had missed it. `hasBrand()` next to it uses `Array.prototype.some`, which is ES5, and is fine. The ES5 runtime guard added alongside it did not catch this because nothing exercised the Client Hints path. It now does, for both bundles. That test builds its hints object from a script evaluated *inside* the vm context rather than assigning one onto it. A first attempt assigned a host-realm object and passed against the live bug: an array created in the host realm keeps the host's `Array.prototype`, so its `find` survives the sandbox's delete. A real browser hands the parser a same-realm array. `pnpm test:types` passed no tarball, and check.mjs exited 1 when the argument was absent, so the advertised command could never run — CI only passed because it packs and invokes the file directly. The argument is now optional: without one the package is assembled from the `files` allowlist in the working tree. `npm pack` cannot be used for this, as package.json carries no `version` until release time and npm refuses to pack without one. Missing build output is reported as such rather than as a type error. Verified by reverting the parser fix: the es5.js Client Hints test fails with the original TypeError while the bundled.js one still passes, which is the correct split. Parse output is unchanged — all four artifacts still agree across the 270-UA corpus, and Client Hints still resolve on modern runtimes. --- src/parser.js | 6 ++- test/acceptance/test-es5-runtime.js | 45 +++++++++++++++++++++++ test/types/check.mjs | 57 ++++++++++++++++++++++++----- 3 files changed, 98 insertions(+), 10 deletions(-) diff --git a/src/parser.js b/src/parser.js index d94aa9b..7412af8 100644 --- a/src/parser.js +++ b/src/parser.js @@ -132,7 +132,11 @@ class Parser { return undefined; } const brandLower = brandName.toLowerCase(); - const brand = this._hints.brands.find( + // `Utils.find`, not `Array.prototype.find`: the latter is ES6, and `es5.js` + // ships no polyfills, so it throws outright on the browsers that bundle + // targets. Every other lookup in this file already goes through the helper. + const brand = Utils.find( + this._hints.brands, b => b.brand && b.brand.toLowerCase() === brandLower, ); return brand ? brand.version : undefined; diff --git a/test/acceptance/test-es5-runtime.js b/test/acceptance/test-es5-runtime.js index 3305b35..00d42aa 100644 --- a/test/acceptance/test-es5-runtime.js +++ b/test/acceptance/test-es5-runtime.js @@ -77,6 +77,51 @@ test('the ES5 sandbox actually strips the modern APIs', (t) => { }); }); +/** + * Client Hints go down a different code path than `parse()` — `isBrandVersion` + * and `getBrandVersion` reach into `_hints.brands` directly — so the checks + * above never touch them. `getBrandVersion` used `Array.prototype.find`, which + * is ES6, and threw on exactly the browsers `es5.js` exists for. + * + * The inputs are built by a script evaluated *inside* the context rather than + * assigned onto it. An array created in the host realm keeps the host's + * `Array.prototype`, so its `find` survives the sandbox's delete and the test + * passes against a bug that is still there. A real browser hands the parser a + * same-realm array, which is what this reproduces. + */ +['es5.js', 'bundled.js'].forEach((file) => { + test(`${file} handles Client Hints on an ES5-only global`, (t) => { + const context = createEs5Context(); + vm.runInContext(fs.readFileSync(path.join(root, file), 'utf8'), context); + + const result = vm.runInContext(` + var ua = ${JSON.stringify(UA)}; + var hints = { + brands: [ + { brand: 'Chromium', version: '131' }, + { brand: 'Google Chrome', version: '131' }, + ], + mobile: false, + platform: 'macOS', + }; + var parser = this.bowser.getParser(ua, false, hints); + ({ + brandVersion: parser.getBrandVersion('Google Chrome'), + missingBrand: parser.getBrandVersion('Firefox'), + hasBrand: parser.hasBrand('Google Chrome'), + hasOtherBrand: parser.hasBrand('Firefox'), + hints: !!parser.getHints(), + }) + `, context); + + t.is(result.brandVersion, '131'); + t.is(result.missingBrand, undefined); + t.true(result.hasBrand); + t.false(result.hasOtherBrand); + t.true(result.hints); + }); +}); + test('bundled.js installs the polyfills it promises', (t) => { // The README tells consumers to reach for bundled.js when they have no // polyfills of their own, so it has to actually populate the environment. diff --git a/test/types/check.mjs b/test/types/check.mjs index 7b4c515..b532e72 100644 --- a/test/types/check.mjs +++ b/test/types/check.mjs @@ -2,7 +2,12 @@ * Type-checks a real consumer against the *packed* package under every module * resolution mode TypeScript offers. * - * node test/types/check.mjs + * node test/types/check.mjs [path-to-bowser-x.y.z.tgz] + * + * With a tarball, the published artifact itself is checked — that is how CI + * runs it. With no argument the package is assembled from the working tree's + * `files` allowlist instead, so `pnpm test:types` works from a plain checkout + * without the caller having to pack first. * * `attw` already runs in CI, but it answers a narrower question: whether the * types *resolve* to the right file for each condition. It does not compile @@ -15,9 +20,11 @@ * named exports because importing them that way throws at runtime, and only a * compile that is expected to *fail* can hold that line. * - * Run against the tarball rather than the repo so the exports map, the - * `types`/`typesVersions` fields and the published file list are all exercised - * exactly as a consumer sees them. + * Either way the package is installed into a real `node_modules` and resolved + * by the normal upward walk, so the exports map, the `types` field and the + * published file list are all exercised exactly as a consumer sees them. A + * tsconfig `paths` mapping would resolve the directory directly and silently + * bypass the exports map, which is the thing most worth testing here. */ import cp from 'node:child_process'; import fs from 'node:fs'; @@ -30,8 +37,8 @@ const repoRoot = path.join(here, '..', '..'); const tsc = path.join(repoRoot, 'node_modules', '.bin', 'tsc'); const tarball = process.argv[2]; -if (!tarball || !fs.existsSync(tarball)) { - console.error('usage: node test/types/check.mjs '); +if (tarball && !fs.existsSync(tarball)) { + console.error(`no such tarball: ${tarball}`); process.exit(1); } @@ -60,8 +67,40 @@ const MUST_NOT_COMPILE = [ const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'bowser-types-')); const modules = path.join(tmp, 'node_modules'); fs.mkdirSync(modules, { recursive: true }); -cp.execFileSync('tar', ['xzf', path.resolve(tarball), '-C', modules]); -fs.renameSync(path.join(modules, 'package'), path.join(modules, 'bowser')); + +/** + * Assembles the package from the repo the way `npm pack` would, for when no + * tarball is supplied. `npm pack` is not an option here: package.json carries + * no `version` (it is stamped at release time), and npm refuses to pack + * without one. Copying the `files` allowlist gives the same tree, so + * `pnpm test:types` works from a plain checkout while CI keeps passing the + * real tarball it already builds. + */ +function assembleFromRepo(dest) { + const manifest = JSON.parse(fs.readFileSync(path.join(repoRoot, 'package.json'), 'utf8')); + // npm always includes these regardless of the `files` allowlist. + const entries = [...manifest.files, 'package.json', 'README.md', 'LICENSE']; + for (const entry of entries) { + const from = path.join(repoRoot, entry); + if (!fs.existsSync(from)) continue; + fs.cpSync(from, path.join(dest, entry), { recursive: true }); + } + const missing = ['es5.js', 'bundled.js', 'bowser.mjs'] + .filter((f) => !fs.existsSync(path.join(dest, f))); + if (missing.length) { + console.error(`missing build output: ${missing.join(', ')} — run \`pnpm build\` first`); + process.exit(1); + } +} + +const pkgDir = path.join(modules, 'bowser'); +if (tarball) { + cp.execFileSync('tar', ['xzf', path.resolve(tarball), '-C', modules]); + fs.renameSync(path.join(modules, 'package'), pkgDir); +} else { + fs.mkdirSync(pkgDir, { recursive: true }); + assembleFromRepo(pkgDir); +} /** Runs tsc over a single file in a project configured for `mode`. */ function typeCheck(mode, fileName, source) { @@ -99,7 +138,7 @@ function hash(s) { } let failures = 0; -console.log(`Type-checking consumers against ${path.basename(tarball)}\n`); +console.log(`Type-checking consumers against ${tarball ? path.basename(tarball) : 'the working tree'}\n`); for (const mode of MODES) { const source = fs.readFileSync(path.join(here, 'fixtures', mode.fixture), 'utf8');