fix(browser): ship ES2017 dist so webpack 4 can parse the SDK - #745
fix(browser): ship ES2017 dist so webpack 4 can parse the SDK#745SomSamantray wants to merge 12 commits into
Conversation
|
|
Still the right fix, and it is now the last piece of #680 that is missing: #749 landed the diagnostic (init tells a react-scripts 4 user why their build dies inside our dist), but this is the one that makes the build actually work. Two things blocking it, neither about the diff itself:
So: rebase onto current main and push, and I will run it. If bench still shrinks after that, then it is worth looking at properly — but I do not expect it to. |
Signed-off-by: Som Samantray <som.samantray@gmail.com>
Signed-off-by: Som Samantray <som.samantray@gmail.com>
Signed-off-by: Som Samantray <som.samantray@gmail.com>
Signed-off-by: Som Samantray <som.samantray@gmail.com>
… astral scripts
The non-\p{L} range fallback (taken only by engines without unicode
property escape support) named Latin, Greek, Cyrillic, Armenian, Hebrew,
Arabic, Hiragana, Katakana, CJK and Hangul, but missed other scripts the
product's locales can hit — Thai, Georgian, and anything outside the
Basic Multilingual Plane. A fragment in one of those scripts on such an
engine was silently treated as letterless and dropped, the same failure
mode `saysSomething` exists to avoid.
Adds Thai and Georgian ranges, and treats any surrogate pair as a letter
(the same over-inclusive direction the function already takes elsewhere
for the rare non-letter astral case, e.g. emoji). Exports the fallback
regex separately so a test can exercise it directly — the primary
\p{L} regex is what every engine running this suite actually takes.
Signed-off-by: Som <Som.samantray@gmail.com>
…APIs `lib` was ES2023 while `target` was ES2017: the target pin stopped newer SYNTAX from reaching dist, but said nothing about newer runtime APIs. `Array.prototype.at` and `Object.hasOwn` (both ES2022) parse fine under any grammar and shipped verbatim in dist, then throw on a genuinely old (pre-2022) browser — silently defeating the point of the ES2017 target for anyone actually running one. `lib` now matches `target` (ES2017), so the compiler itself catches this class of gap instead of a human noticing at runtime. `Object.hasOwn` in `state-select.ts` becomes `Object.prototype.hasOwnProperty.call`; every `.at(-1)`/`.at(0)` in source and tests becomes direct indexing (tests share a new `at()` test-support helper, since they cannot use the real `.at()` either — this package's tsconfig covers both). One deliberate, minimal, documented exception: `WeakRef` (ES2021) has no downlevel-able equivalent, so it is allowed back in via the narrower `ES2021.WeakRef` lib fragment rather than reopening the whole ES2023 surface. Universally supported in evergreen browsers since 2021, well ahead of the webpack-4-parseable syntax floor this pin targets. Signed-off-by: Som <Som.samantray@gmail.com>
The stated purpose of this fix — "a webpack 4 app can install @reticlehq/browser again with no bundler-config edits" — did not hold: zod (a real runtime dependency of @reticlehq/core, so of every consumer of @reticlehq/browser) ships `??`/`?.` in its OWN dist from 3.23.0 onward, including at the exact floor this repo previously declared (`^3.24.1`). A webpack 4 app installing today's @reticlehq/browser would still fail to parse zod's bundled code — the same failure issue reticlehq#680 fixed for this repo's own packages, reopened one dependency down where no local check could see it. Pins `zod` to `3.22.4`, the last version confirmed (by fetching and scanning each candidate release) free of this syntax, with `zod-to-json-schema` pinned to the matching peer-compatible version so the schema generation build keeps working. A dependabot `versions` ignore blocks any future zod bump past 3.22.4 from landing unreviewed — the existing semver-major-only ignore would have let a routine "minor-and-patch" grouped PR sail right through the regression. Also extends `legacy-syntax-guard.test.ts`: it now resolves and scans whatever zod @reticlehq/core actually installs (not a hardcoded path), so a future bump past the pin fails CI immediately instead of silently shipping. And it adds a real acorn-based parse of dist at the actual webpack-4 ceiling (ES2019 — issue reticlehq#680 was specifically ES2020+ nullish coalescing, optional chaining, and logical assignment; webpack 4's own bundled acorn already handles ES2018/2019 constructs like object spread), rather than only grepping for five known tokens. That parse step is what caught this: zod 3.22.4's own dist uses object spread, which the existing grep never checked for and which turned out to be correctly outside the real webpack-4 floor once verified against the right ceiling. Signed-off-by: Som <Som.samantray@gmail.com>
…oint CI failed: @reticlehq/server's build broke with TypeScript errors like "Type 'ZodObject<...>' is missing ... '~standard', '~validate'" wherever server composed a core-exported schema with its own zod (`.extend()`, `z.array()`, a generic `ZodTypeAny` parameter, embedding one inside a `z.object()`). Pinning core's zod to 3.22.4 (previous commit) means core and server now resolve genuinely different zod module instances — `@modelcontextprotocol/sdk` needs `^3.25`, core needs `<3.23.0` — so a core-built schema is a different, incompatible TypeScript nominal type from server's, even though it is a real, working schema at runtime. `schema-interop.ts` re-types a core schema at composition points (`asServerZodObject`, `asServerZodType<Output>`) so TypeScript accepts it. Applied at `intent-shard.ts`'s `.extend()`, `capsule-store.ts`'s `z.array()`, `session-journal.ts`'s generic parameter, and `predicate-schema.ts`'s embedded `ElementQuerySchema`. That alone was not enough: `nestedKeysOf` in `predicate-schema.ts` did `instanceof z.ZodObject` to introspect a predicate's nested fields, and this — like every `instanceof` check — compares prototypes. A core-built schema is never `instanceof` server's `ZodObject`, `ZodLiteral`, etc., even after the type-cast, so the check silently returned `[]` for `element`'s `query` field and the grammar the tool surface advertises lost every nested field (`role`, `by`, `value`, ...). `schema-interop.ts` now also exports `isZodObject`/`isZodOptional`/`isZodNullable`/ `isZodDefault`/`isZodEffects`/`isZodLiteral`, duck-typed on zod's own internal `_def.typeName` tag (identical across every zod 3.x build), and `predicate-schema.ts`'s five `instanceof z.Zod*` checks are rewritten to use them. The deepest instance of this same bug was not in this repo's code at all: zod's own `ZodRecord.create(first, second, third)` decides its one-arg-vs-two-arg overload with `second instanceof ZodType`. Calling `z.record(z.string(), IntentRecordSchema)` — `IntentRecordSchema` being a core-instance `ZodObject` from `IntentSchema.extend(...)` — silently failed that check and fell into the ONE-arg branch, discarding `IntentRecordSchema` as an options object and making every stored intent's value schema resolve to plain `z.string()`. Every write kept succeeding (nothing threw), but every read back through `IntentShardSchema.parse()` failed and returned the empty fallback: `get()` returned `null` for a record that had just been written, and `why`/`binding`/`source` all vanished on any second write. Confirmed against a clean `main` checkout (all 11 tests pass there, where core and server share one zod instance) and fixed by dropping the redundant explicit key schema — `z.record(IntentRecordSchema)` — which has no such branch and defaults to string keys regardless. Verified: 6280/6280 server tests pass (previously 6271/6280, the 9 failures spanning `intent-shard-store.test.ts`, `predicate-shape.test.ts`, `predicate-grammar.test.ts`, and `predicate-parse.test.ts`), `tsc -b` clean, and a direct reproduction script confirms `IntentShardStore.record()`/`.get()` now round-trip `why`/`binding`/`source` correctly across repeated writes. Signed-off-by: Som <Som.samantray@gmail.com>
CI's format:check caught what local pnpm lint/typecheck/test:unit did not — this repo's own lesson from CLAUDE.md's gates section, restated: format:check is the one thing verify enforces that lint does not run. Signed-off-by: Som <Som.samantray@gmail.com>
Rebasing onto main pulled in flow-expect-grammar.ts/flows.ts changes that call describeFlowZodFailure(result.error) where result comes from FlowFileSchema.safeParse — a core-built schema. With core's zod pinned below server's (see schema-interop.ts), that error is core's ZodError instance, not server's, and exactOptionalPropertyTypes makes the two nominally incompatible — tsc -b failed the build. Adds asServerZodError alongside asServerZodType/asServerZodObject and applies it at both call sites (and the test that exercises the same path directly), consistent with how every other cross-instance zod value in this file is already bridged. Signed-off-by: Som Samantray <som.samantray@gmail.com>
05459a3 to
11ebbe2
Compare
|
Done the required changes! |
|
Reviewed and merging. Sorry this sat five days fully green — including the 28-minute Windows install gate — waiting on someone to look at it. I only found it because I had just written a worse version of it (#867, now closed as a duplicate of this). That gave me an unusually direct way to review yours: three things you did that I did not, and one of them is the difference between fixing #680 and only appearing to. Fixing
Scanning We independently landed on parsing at ES2019 rather than grepping for operators, which I take as a good sign about the check — a regex version I tried first produced two false failures on output that was already correct, because the hits were inside comments. Thank you for this. It is a better fix than the one I wrote knowing the codebase. |
divshekhar
left a comment
There was a problem hiding this comment.
Approved. See my review comment above: the core-package half is the difference between fixing #680 and only appearing to, and the /\p{L}/u handling is a limitation I did not know about and would have shipped broken.
|
Approved, and I cannot merge it — worth saying exactly why so it does not sit another five days. This PR touches That is a limit on me, not a problem with your change. Two ways forward, your choice:
Everything else is ready: every check green, including both install gates, and approved. Nothing further is wanted from you unless you pick option 1. |
What & why
A webpack 4 (
react-scripts4) app can install@reticlehq/browseragain with no bundler-config edits. The SDK compiled to ES2022, which emits??,?., and??=verbatim, and webpack 4 cannot parse any of them — so the app failed to compile before a session could connect.Closes #680
How it was verified
pnpm buildof browser + core, then everydistfile (144, including.cjs) parsed with an ES2017 grammar: 0 unparseable. Before the fix the same scan failed across both packages.legacy-syntax-guard.test.tsfails on the pre-fix emit (3 fail, proven via stash-revert-rebuild) and passes on the fixed one — now also: a real acorn parse ofdistat the actual webpack-4 ceiling (ES2019, not just an ES2017 token grep), and a scan of whateverzod@reticlehq/coreactually resolves.pnpm lint && pnpm typecheck && pnpm test:unitclean on@reticlehq/browserand@reticlehq/core./\p{L}/uliteral, since tsc never downlevels regex bodies — is built from a string with a letter-block fallback, now covering Thai, Georgian and every astral-plane script, with a dedicated test exercising the fallback regex directly (not just the primary\p{L}path every engine running the suite actually takes).CI findings addressed
The first CI run failed the build on several jobs: the ES2017 target constrains test files too, and four pre-existing tests used post-ES2017 syntax (
/sflag, BigInt literals). Those are rewritten in an ES2017-compatible form with behavior unchanged (tests run on modern Node and never ship). A clean from-scratch build now succeeds on all targets.The remaining
package-qualityfailure was the SDK size budget. The ES2022 build packs at ~774KB, and the ES2017 floor packs at ~1063KB because class fields and private members must downlevel to WeakMap/assign helpers below ES2022. No target avoids that cost (ES2018 measures the same), so the budget moved to 1100KB under the gate's own protocol — the reason is in this diff.Pinning
zodin@reticlehq/coreto3.22.4(below@reticlehq/server's^3.24.1, needed for@modelcontextprotocol/sdk's peer range) broke@reticlehq/server's own build:packages/servercomposes a handful of core-exported zod schemas with its own zod instance (.extend(),z.array(), embedding one in az.object()), and the two zod versions are genuinely separate module instances once they diverge — TypeScript sees them as incompatible nominal types, and worse,instanceofchecks against either version's own classes fail across the boundary even though the schema works correctly at runtime. This surfaced in three layers: TypeScript errors at the composition sites (fixed with a smallschema-interop.tsbridge), a predicate-grammar introspection helper that usedinstanceof z.ZodObjectand silently returned no nested fields for a cross-instance schema (fixed with duck-typed_def.typeNamechecks), and — the one that actually corrupted data — zod's ownZodRecord.create()deciding its one-arg-vs-two-arg overload withsecond instanceof ZodType, soz.record(z.string(), IntentRecordSchema)silently discarded the value schema and made every stored intent parse against plainz.string(). Writes never threw; reads silently came back empty. Fixed by dropping the redundant explicit key schema (z.record(IntentRecordSchema)), which has no such branch. All 6280 server tests pass, verified against a cleanmaincheckout to confirm this was a genuine regression from the zod pin and not pre-existing.Review findings addressed (this update)
All five findings from the first review pass are now fixed, not just documented:
HAS_LETTER_FALLBACKis exported and tested directly, since every engine running this suite takes the primary\p{L}path and would never exercise the fallback ranges otherwise..at(),Object.hasOwn) —libnow matchestarget(ES2017), so the compiler itself catches this class of gap instead of a human noticing at runtime. Both call sites are rewritten to ES2017-safe equivalents;WeakRef(ES2021, no downlevel-able equivalent) is kept via a narrow, documentedES2021.WeakReflib exception rather than reopening the whole ES2023 surface.zodfloor — this was real, not hypothetical:zod's own dist ships??/?.from 3.23.0 onward, including at the range this PR previously declared (^3.24.1). A webpack 4 app installing today's@reticlehq/browserwould still fail on zod's bundled code.zodis now pinned to3.22.4(last version confirmed clean by fetching and scanning each candidate release),zod-to-json-schemapinned to the matching peer version, and a dependabotversionsignore blocks a future bump past the pin from landing unreviewed.legacy-syntax-guard.test.tsnow actually parsesdistwithacornpinned to the real webpack-4 ceiling (ES2019 — issue @reticlehq/browser needs a legacy-compatible build for webpack 4 / react-scripts 4 #680 was specifically ES2020+ nullish coalescing/optional chaining/logical assignment; webpack 4's bundled acorn already handles ES2018/2019 constructs like object spread), instead of only grepping five known tokens. This is what caught the zod issue above: zod's dist uses object spread, which the old grep never checked and which is correctly inside the real webpack-4 floor once verified against the right ceiling rather than an arbitrarily strict one.target/libskew — resolved as a side effect of the runtime-API fix above:libandtargetare now both ES2017 (plus the one documentedWeakRefexception), so there is no more skew between what this repo emits and what it type-checks against.Rebase onto main (this update)
Rebased onto current
mainto resolve a conflict inpackages/server/src/intent/intent-shard.ts: main had only dropped theexportkeyword on file-local symbols (#557's export cleanup) since this branch diverged, so the resolution keeps this PR's interface +schema-interopfix and drops the now-redundant export to match.The rebase pulled in unrelated
mainchanges toflow-expect-grammar.ts/flows.tsthat calldescribeFlowZodFailure(result.error)against a core-builtFlowFileSchema.safeParseresult. With core pinned to an olderzod(this PR), thatZodErroris a different module instance from server's own, andexactOptionalPropertyTypesmakes the two nominally incompatible —tsc -bfailed post-rebase. AddedasServerZodErrortoschema-interop.ts(same pattern as the existingasServerZodType/asServerZodObjectbridges) and applied it at both call sites plus the test that exercisesdescribeFlowZodFailuredirectly.Re-verified clean after rebase: full monorepo
pnpm build,pnpm lint,pnpm typecheck,pnpm format:checkall pass;pnpm test:unitis 669 files / 6693 tests green (server) plus the rest of the monorepo;legacy-syntax-guard.test.ts's real acorn/ES2019 parse ofdiststill passes.The
benchgate failure from the previous CI run was diagnosed (by both reviewer and me) as a stale-branch artifact unrelated to this diff's code — the same "coverage shrank" signature independently seen on an unrelated PR, caused by MCP-server-boot timeouts under CI load, not by anything this PR touches. Expected to go green on this rebased head.Gates run
pnpm lint && pnpm typecheck && pnpm test:unit(~2 min — always)pnpm test:e2e(~8 min) — touched the tool surface,packages/core, an observer, or telemetrypnpm gate:install(~15 min) — touchedreticle init,vite-plugin,next, orbabel-pluginpnpm test:e2e:desktop(~3 min) — touchedpackages/electron,packages/tauri, or desktop captureChecklist
git commit -s) — CI's DCO check fails the PR without it. Already pushed?git rebase --signoff origin/main && git push --force-with-leaseany, no free strings (wire strings live in@reticlehq/core), no non-null!console.logor internal tracking codes left in the diffCHANGELOG.mdupdated if this is user-facing (entry under[Unreleased])docs/telemetry.md) and are covered by a testUnapplied review findings
None — all five findings from the first review pass are resolved above.