impr(tinyest-for-wgsl): Separate acorn and babel parsers - #2932
impr(tinyest-for-wgsl): Separate acorn and babel parsers#2932cieplypolar wants to merge 18 commits into
Conversation
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased | ❔ Unknown |
|---|---|---|---|
| 0 | 325 | 0 | 0 |
No notable changes.
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.91, 1.85, 3.77, 5.91, 6.77, 11.59, 24.39, 26.17]
line [0.96, 1.94, 3.81, 6.56, 7.13, 12.40, 22.82, 23.92]
line [0.86, 1.77, 3.94, 6.26, 7.30, 11.50, 23.70, 25.47]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.33, 0.48, 0.68, 0.83, 1.07, 1.11, 1.31, 1.45]
line [0.27, 0.49, 0.71, 0.84, 1.11, 1.17, 1.35, 1.54]
line [0.29, 0.57, 0.67, 0.80, 1.10, 1.17, 1.26, 1.43]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.79, 2.08, 3.23, 6.72, 12.13, 25.46, 55.03, 109.88]
line [0.90, 2.07, 3.28, 6.31, 11.86, 24.82, 56.03, 110.82]
line [0.80, 2.10, 3.13, 6.67, 11.50, 24.97, 55.07, 108.31]
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — two minor things worth a look.
Reviewed changes — This PR splits tinyest-for-wgsl's single shared transpiler map into two parser sets. A new baseTranspilers map holds the logic common to both AST shapes, while acornSpecificTranspilers / babelSpecificTranspilers cover the differing node types (acorn Literal vs babel NumericLiteral/StringLiteral/BooleanLiteral/BigIntLiteral). unwrapToFunction / param parsing move out of parsers.ts into a new functionParts.ts, and the public transpileFn / transpileNode signatures now require a mandatory { ast: 'acorn' | 'babel' } options arg, dispatching to the correct parser set. unplugin-typegpu is updated to pass { ast: 'babel' }.
- Shared transpilers (
Program,BlockStatement,MemberExpression, control flow, etc.) moved verbatim intobaseTranspilers— behavior preserved. - Acorn and babel specific literal / object-expression transpilers separated by AST shape; runtime has no runtime dependency on either parser (all dispatch is by
node.typestring). - Improved error handling: Acorn computed object keys are now explicitly rejected (previously silently mis-handled), and
nullliterals now throw instead of lowering to"0". - New
Transpilers<TNode>/Transpile<TNode>generics andTranspilationOptions/AstKindtypes;acorn/@babel/typesmoved to optional peerDependencies. - Tests updated to pass
{ast}and added coverage for computed object keys and bigint/binary literals.
I verified the split is behavior-preserving: shared handlers are equivalent to the originals, pnpm --filter tinyest-for-wgsl run test:types passes, and both the parsers.test.ts (19 tests) and unplugin-typegpu obfuscation.test.ts (24 tests) suites pass.
ℹ️ prepublishOnly references a devDependency this PR removed
prepublishOnly: "tgpu-dev-cli prepack" remains in package.json, but @typegpu/tgpu-dev-cli is dropped from devDependencies in this diff. Because pnpm only exposes a package's own declared deps on its node_modules/.bin path, publishing this package will fail to resolve the tgpu-dev-cli binary. Either keep the devDependency or drop the now-unused prepublishOnly step alongside the removal.
Technical details
# `prepublishOnly` and the removed `tgpu-dev-cli` devDependency
## Affected sites
- `packages/tinyest-for-wgsl/package.json` — `"prepublishOnly": "tgpu-dev-cli prepack"` (unchanged context) vs `@typegpu/tgpu-dev-cli` removed from `devDependencies` (this diff, hunk `@@ -58,11 +58,22 @@`).
## Required outcome
- `tinyest-for-wgsl` can be published without "command not found" for `tgpu-dev-cli`.
## Suggested approach
- Restore `@typegpu/tgpu-dev-cli: "workspace:*"` in `devDependencies`, or drop the `prepublishOnly` script if `prepack` is no longer needed for this package.ℹ️ Optional peer types vs public .d.ts
acorn and @babel/types are now optional peerDependencies, but both are referenced as import type namespaces in the public entry points (transpileFn/transpileNode overloads accept acorn.AnyNode and babel.Node). Since this package is published publicly, any TypeScript consumer must have both packages installed to typecheck against the published .d.ts, even when they only use the acorn or babel path — optional peers mean npm won't prompt them to add the missing one. Worth confirming that dual-install requirement is intended before merge.
Technical details
# Optional peers still referenced by public signatures
## Affected sites
- `packages/tinyest-for-wgsl/src/parsers.ts` — `transpileFn`/`transpileNode` overloads on `acorn.AnyNode` / `babel.Node`.
- `packages/tinyest-for-wgsl/src/types.ts` — `JsNode = babel.Node | acorn.AnyNode`.
## Required outcome (open question for the human)
- Decide whether optional peers are the right stance given the `.d.ts` requires both for type resolution. Fine to keep; just confirm external consumers won't hit `Cannot find module 'acorn'` during typecheck.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Since the prior review, the only change is in packages/tinyest-for-wgsl/package.json (plus the pnpm-lock.yaml regen): @typegpu/tgpu-dev-cli is restored to devDependencies.
- Re-added
@typegpu/tgpu-dev-cli: "workspace:*"todevDependencies, resolving the prior finding thatprepublishOnly: "tgpu-dev-cli prepack"referenced a binary no longer present on the package'snode_modules/.bin.
The prior informational note about optional peers (acorn / @babel/types) still being referenced as types in the public .d.ts remains an open design question for the author, not a blocker, and is intentionally left as-is.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Since the prior review (44c2c5174, approved), the delta covers the unreviewed "comments" and "import cleanup" commits plus the latest 1de82e1 "cleanup": the acorn object-property rejection was split into distinct computed / "Unsupported non-computed" errors with an added guard for Literal keys whose raw is null or non-regex, TranspilationResult is now exported from the package entry point, and the dualTest test helper dropped its dependency on internal types.ts.
- Split the acorn
ObjectExpressionkey rejection so computed and non-computed invalid keys throw distinct, clearer errors; the newLiteralraw === null/regexguard is dead for realistic input (such keys aren't valid JS) and behavior-preserving for string/numeric keys. - Exported
TranspilationResultfromindex.ts; it only referencestinyesttypes, so no additionalacorn/@babel/typessurface leaks into the public.d.ts. - Reworked the
dualTesthelper intests/helpers.tsto importTranspilationResultfrom the published package and use the publicNode | acorn.AnyNodebound instead of internaltypes.ts. - Reordered the
binary bigintsandrejects computed object propertiestests with no assertion changes.
parsers.test.ts (19 tests) passes, including both reordered cases. The optional-peer design question previously noted (acorn / @babel/types referenced as types in public .d.ts) is unchanged by this delta and remains an open, informational consideration.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Since the prior review (1de82e1, approved), the delta is the merge commit 53f47c7 plus main's history it absorbed. The PR-authored part adds null literal support to both transpiler sets and a regression test; the rest is main content (2d43507f6 version bumps including tinyest-for-wgsl 0.4.1, and 7546d013d feat: allow null at comptime).
- Acorn
Literalwithnode.raw === 'null'now returns[NODE.nullLiteral](previously threw'null is not representable in WGSL.'), and babel gained aNullLiteral()handler (previously fell through to'Unsupported JS functionality: NullLiteral'). - Added a dual-parser
parses nullregression test asserting the AST snapshot[0,[[13,"a",[106]]]]— it fails on both parsers under the pre-change behavior. - Confirmed
NODE.nullLiteral(106) exists intinyestand is handled downstream (wgslGenerator.ts:1051emits anullconstant), so the change aligns the parser with main's new comptime-null feature rather than emitting an unrepresentable node.
parsers.test.ts (20 tests) and obfuscation.test.ts (25 tests) pass; pnpm --filter tinyest-for-wgsl run test:types passes. The long-standing informational note about optional peers (acorn / @babel/types) referenced as types in the public .d.ts remains open and unchanged by this delta.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| export function dualTest( | ||
| test: <TNode extends Node | acorn.AnyNode>( | ||
| p: (code: string) => TNode, | ||
| transpileFn: (node: TNode) => TranspilationResult, |
There was a problem hiding this comment.
Is this ever any other function?
There was a problem hiding this comment.
could you elaborate?
There was a problem hiding this comment.
Important
Two blocking regressions landed in the legacy commit (13cbd78). The restored single-arg transpileFn signature breaks unplugin-typegpu's test:types gate, and the @babel/types import flipped to a value import re-introduces a runtime dependency on an optional peer.
Reviewed changes — Since the prior review (53f47c7), the legacy commit restores the pre-PR single-arg transpileFn/transpileNode as deprecated and adds dedicated transpileFnAcorn/transpileFnBabel/transpileNodeAcorn/transpileNodeBabel, routing through a new createParser('acorn' | 'babel' | 'legacy') with a lazily-created merged legacy parser.
- Restored the single-arg
transpileFn/transpileNodesignatures as deprecated compat shims that dispatch to a merged{...babelTranspilers, ...acornTranspilers}set with a combinedObjectExpressionhandler (createLegacyTraspilers). - Added the dedicated
transpileFnAcorn/transpileFnBabel/transpileNodeAcorn/transpileNodeBabelexports; droppedAstKind/TranspilationOptionsfrom the public type surface. - Added a
legacy transpileFntest suite (mixed identifier/string/numeric/bigint keys; computed/spread/method rejections) and reworkeddualTestto thread the transpile-fn and use the dedicated entries. - Switched
parsers.ts's@babel/typesimport from type-only to a value import (root cause of the runtime-dep regression).
ℹ️ Nitpicks
createLegacyTraspilersis misspelled (missing "n"); private and unreleased, but worth a tidy while here.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
7bfa8f1 to
2493b19
Compare
There was a problem hiding this comment.
Important
Prior review finding (1) is only half-addressed: the @babel/types value-import regression is cleanly reverted to import type * as babel (finding 2), and all unplugin src/ callers now use transpileFnBabel — but the obfuscation test helper still calls the single-arg legacy transpileFn with a second { ast: 'babel' } arg, so unplugin-typegpu/test:types remains red.
Reviewed changes — The fix commit (2493b19) addresses both must-address findings from the prior review (13cbd78).
- Reverted
parsers.ts:1fromimport * as babelback toimport type * as babel, removing the runtime side-effect import of the optional peer@babel/typesfrom the built package. Verified it matchestranspilers.ts/functionParts.ts. - Migrated unplugin consumers to the dedicated babel entry:
core/common.ts(transpilenow callstranspileFnBabel(rootNode)withParameters/ReturnType<typeof transpileFnBabel>),babel.ts,core/factory.ts, andcore/obfuscate.ts.
test/obfuscation.test.ts was not part of this delta; its line 178 is what still trips the type gate (see inline comment). pnpm --filter tinyest-for-wgsl run test:types passes; pnpm --filter unplugin-typegpu run test:types fails with TS2554 at obfuscation.test.ts(178,31).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
0875f09 to
ad791e6
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — Since the prior Pullfrog review (2493b19 tree), the branch was force-pushed (history rewritten; 44245d79 "legacy" now sits directly on the 53f47c73 merge, net tree content preserved) and merged with main (ad791e6e). The only PR-authored delta is the obfuscation.test.ts migration that resolves the standing must-address type-gate finding.
- Migrated
test/obfuscation.test.tsoff the deprecated single-argtranspileFnwrapper to the dedicatedtranspileFnBabelentry at all 18 call sites, removing the{ ast: 'babel' }second argument that tripped theTS2554gate; imports tidied toimport type { ArrowFunctionExpression }. - Verified the fix lands the whole suite:
pnpm --filter unplugin-typegpu run test:typespasses (previously failed),tinyest-for-wgsl test:typespasses, runtimeobfuscation.test.ts25/25 andparsers.test.ts24/24 pass. Thepnpm-lock.yamldelta is main-merge noise only, no net PR change.
The long-standing informational note about optional peers (acorn / @babel/types) being referenced as types in the public .d.ts remains an open design question for the author, unchanged by this delta and not a blocker.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
No description provided.