Skip to content

fix(browser): ship a build react-scripts 4 / webpack 4 can parse - #868

Closed
Bhumika-1432006 wants to merge 1 commit into
reticlehq:mainfrom
Bhumika-1432006:fix/browser-legacy-build
Closed

fix(browser): ship a build react-scripts 4 / webpack 4 can parse#868
Bhumika-1432006 wants to merge 1 commit into
reticlehq:mainfrom
Bhumika-1432006:fix/browser-legacy-build

Conversation

@Bhumika-1432006

Copy link
Copy Markdown
Contributor

What & why

@reticlehq/browser's dist/index.js compiled to the monorepo's shared ES2022 target, so optional chaining (?.), nullish coalescing (??), and nullish-coalescing assignment (??=) went out untranspiled — including right at the top of index.ts, in a line that runs unconditionally at module load (globalStore.__reticleInstance ??= new Reticle()).

react-scripts 4 / webpack 4 excludes node_modules from Babel, so any app still on that stack fails to even parse the SDK the moment it's imported — before a dev session can ever connect, with no diagnostic pointing at why. A maintainer had already landed the diagnostic half of this (init now detects react-scripts@<5 and explains what's about to happen), and narrowed the issue to exactly this remaining piece — the actual fix, calling it "a good candidate for a contributor: the change is scoped to one package's build config."

What changed

  • packages/browser/tsconfig.json now overrides target to ES2019 (the shared tsconfig.base.json stays ES2022 for the rest of the monorepo — nothing else moves, since only this package's dist/ is parsed by an end user's own bundler rather than just Node).
  • Three test files (stores.test.ts, serialization.test.ts, transport.security.test.ts) used a BigInt literal (2n) to exercise BigInt handling in the redaction/serialization path. That's syntax TypeScript refuses to emit below ES2020 — unrelated to whether it gets downleveled, BigInt precision just can't be polyfilled. Swapped for BigInt(2), same runtime value, so the whole package (tests included, since tsc -b type-checks them under include) compiles under the lowered target.
  • Added packages/browser/src/build-target.test.ts: it reads this package's actual tsconfig.json via the TypeScript compiler API and compiles a snippet using all three operators through it, asserting none survive. This is a real regression test, not a manual check — if someone bumps the target back up later (or the shared base tsconfig changes in a way that leaks through), this fails in CI instead of in a user's silent build.

Manually confirmed the built output is clean: grep -c '?\.' dist/index.js, grep -c '??=' dist/index.js both come back 0 after a full rebuild.

How it was verified

  • packages/browser/src/build-target.test.ts written first against the unmodified tsconfig (target ES2022) — fails as expected, showing ?. / ??= surviving in the compiled snippet.
  • Applied the tsconfig.json change — test goes green.
  • Full package rebuild (tsc -b --force) — clean, and dist/index.js verified free of all three operators by direct grep.
  • Full packages/browser unit suite: 134 test files / 1271 tests, all passing (including the three BigInt-literal fixes and the new test).
  • tsc -b --force (typecheck) — clean, no errors.
  • eslint on every changed file — clean.
  • prettier --check on every changed file — clean.

Gates run

  • pnpm lint && pnpm typecheck && pnpm test:unit (~2 min — always) — ran scoped to @reticlehq/browser: full 1271-test suite green, typecheck clean, lint clean, prettier clean.
  • pnpm test:e2e (~8 min) — touched the tool surface, packages/core, an observer, or telemetry
  • pnpm gate:install (~15 min) — touched reticle init, vite-plugin, next, or babel-plugin
  • pnpm test:e2e:desktop (~3 min) — touched packages/electron, packages/tauri, or desktop capture
  • None of the above tiers apply to this change — it's a build-config change (target) plus test fixture syntax, no tool surface / core / telemetry / install-path / desktop code touched.

Checklist

  • Every commit is signed off (git commit -s)
  • Tests added/updated (RED → GREEN); the change is covered by a test that would fail without it
  • No any, no free strings, no non-null !
  • No console.log or internal tracking codes left in the diff
  • Each changed file is under the 1000-line cap
  • CHANGELOG.md updated under [Unreleased]
  • Not security-affecting — build-config and test-syntax only

Closes #680.

🤖 Generated with Claude Code

dist/index.js compiled to the monorepo's shared ES2022 target, which meant
optional chaining, nullish coalescing, and nullish-coalescing assignment
(??=) went out untranspiled. react-scripts 4 / webpack 4 excludes
node_modules from Babel, so any app on that stack failed to compile the
moment it imported the SDK -- before a dev session could ever connect, and
with no diagnostic pointing at why.

packages/browser's own tsconfig.json now overrides target to ES2019,
downleveling all three operators. Only this package moves; the rest of the
monorepo runs on a Node version that never had this problem, so its target
stays ES2022.

Three test files used a BigInt literal (2n) to exercise BigInt handling in
serialization -- syntax TS refuses to emit below ES2020, unrelated to
whether it's downleveled. Switched to BigInt(2), same runtime value, so the
lowered target can compile the whole package including its tests.

Added a regression test that compiles a snippet through this package's
actual tsconfig and asserts optional chaining/nullish coalescing/??= don't
survive, so a future bump of the target back up fails in CI instead of in a
user's silent build failure.

Closes reticlehq#680.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Bhumika <guravbhumika808@gmail.com>
@divshekhar

Copy link
Copy Markdown
Contributor

Thank you — and I am sorry, because three of us built this independently today. Closing in favour of #745, which has been open since 3 September, is fully green including both install gates, and is the most complete of the three.

I wrote one of the other two (#867) and closed it for the same reason I am closing this one, so this is not a judgement about your work over mine.

The gap all three of our browser-only versions share: @reticlehq/browser imports @reticlehq/core, so core ends up in the user's bundle too. I checked after reading #745 — core's dist fails an ES2019 parse in 10 files (constants.js, impact.js, daemon-registry.js, contract-fingerprint.js, finding-fingerprint.js, …). Lowering only the browser target closes #680 on paper while a webpack 4 app still fails to compile, which is worse than leaving it open.

And one thing none of us caught except #745: /\p{L}/u. tsc never downlevels regex bodies, whatever the target is — so a Unicode property escape survives the target change and still fails to parse. #745 builds it from a string with a letter-block fallback.

Your diagnosis is exactly right, including the detail that matters most: globalStore.__reticleInstance ??= new Reticle() runs unconditionally at module load, so this is not a code path a user can avoid. That sentence is the clearest statement of the severity in any of the three PRs, and I have quoted it on the issue.

If you would like something adjacent that is genuinely open: @reticlehq/browser currently publishes its compiled test files — there are 10+ *.test.js in dist and files: ["dist"] ships them. That is real bloat in a package users install, it is nobody's territory right now, and it is a clean small PR.

@divshekhar divshekhar closed this Sep 8, 2026
@Bhumika-1432006

Copy link
Copy Markdown
Contributor Author

No apology needed at all — three people converging on the same fix the same day just means the issue was well-scoped and clearly worth doing. Thanks for the detailed writeup rather than a plain "closing, see #745."

The two things I missed are both real gaps in my testing, not just bad luck: I verified dist/index.js was clean but never checked whether @reticlehq/core's own output survives the same parse, and I had no test anywhere that would have caught a \p{L} regex surviving a target change (that one especially — tsc not touching regex bodies regardless of target isn't something I'd have thought to check for without hitting it). Good to know for next time: a "does this actually parse under webpack 4" check needs to walk the whole dependency chain, not just the one package's own dist, and needs an actual parse attempt (not just a grep for known-bad tokens) to catch regex-level issues a syntax grep can't see.

Glad #745 is the more complete fix and it's green on both install gates already.

I'll take a look at the compiled-test-files-in-dist cleanup you mentioned — thanks for pointing at something genuinely open instead of leaving me to go looking.

@Bhumika-1432006

Copy link
Copy Markdown
Contributor Author

Looked into the compiled-test-files suggestion before opening anything, and I don't think there's a live bug here — reporting back rather than filing a no-op PR.

`scripts/prepare-dist.mjs` already strips every `.test.` file (and source maps) as part of `prepack`, and every real publish path runs through it: `RELEASING.md`'s actual release step is `pnpm -r publish` (which fires `prepack` per-package), and the install gate's local-registry simulation (`apps/e2e/install-gate.mjs`) uses the exact same `pnpm -r publish --registry ... --no-git-checks` command. So `files: ["dist"]` is true, but by the time `npm pack`/`publish` actually reads that field, `prepack` has already run and `dist` is already clean.

Confirmed empirically rather than just by reading the script — `npm pack @reticlehq/browser@latest --dry-run`, same for `@reticlehq/core` and `@reticlehq/react`, and none of the three currently-published tarballs contain a single `.test.` file. The bloat is real in a bare `pnpm build` output (that command alone doesn't run prepack), but that's local dist state, not what ships.

If you were thinking of something I'm not seeing — a different publish path that skips lifecycle scripts, or a version predating `prepare-dist.mjs` — let me know and I'll take another look. Otherwise this one looks already handled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@reticlehq/browser needs a legacy-compatible build for webpack 4 / react-scripts 4

2 participants