refactor(plugins): share small AST helper utilities#1754
Conversation
commit: |
AST-heavy plugin transforms currently duplicate loose node guards, child traversal, binding-pattern extraction, and Identifier or Literal name extraction. That makes PR 1736 larger than it needs to be and increases the chance that each transform grows its own slightly different syntax helpers. Extract the small parser-shape helpers into a local plugin utility module and migrate import-meta-url plus optimize-imports to use it. Scope-aware transform logic remains local to each plugin, so this preserves runtime behaviour while creating a narrow reuse point for the dynamic-request transform.
NathanDrake2406
force-pushed
the
nathan/ast-utils
branch
from
529b9ed to
4636459
Compare
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Clean, low-risk extraction. I verified the validation steps locally: vp test run tests/import-meta-url.test.ts tests/optimize-imports.test.ts tests/remove-console.test.ts (117 passed) and vp check on the three files (format/lint/types clean).
I traced the subtle semantic changes between the old local helpers and the new shared ones, and they are behaviorally equivalent (or slightly more correct) for the real AST shapes produced by Vite's parseAst:
isNodeLike→isAstRecord: stricter — old returned true for any non-null, non-array object; new requires a stringtype. All call sites pass real AST nodes (which havetype), and the recursive walkers (visit,recordReads) bail on non-nodes anyway, so the effective result is unchanged.forEachAstChildadds"parent"to the skip set: a behavior change vs. the old inline loops, but defensive and harmless sinceparseAst(oxc) ASTs don't carryparentbackrefs. Nice guard against accidental cycles.findDirectivePrologueEndhashbang: good catch keeping the rawtypeof ... === "object"+Reflect.getcheck here rather thanisAstRecord(hashbang). A hashbang node may not have a stringtype, soisAstRecordwould have regressed this. Worth a one-line comment so a future cleanup doesn't "simplify" it back intoisAstRecord.astName→getAstName: stricter (now requirestype === "Identifier"for thenamepath) but equivalent for ESTree Identifier/Literal/StringLiteral shapes the callers actually receive.
No leftover references to the removed NodeLike/isNodeLike/astName helpers anywhere in the source. Approving in spirit — only optional nits below.
| end: number; | ||
| }; | ||
|
|
||
| const SKIP_CHILD_KEYS = new Set(["type", "parent", "loc", "start", "end"]); |
There was a problem hiding this comment.
Optional: the old inline walkers in import-meta-url.ts skipped only type/start/end/loc and did not skip parent. Adding parent here is a safe, defensive change (avoids cycles if a parent backref ever exists), but it is a behavioral difference worth a short comment so the intent is clear, e.g. // skip backrefs + positional metadata.
| return isAstRecord(value) && value.type === "Identifier" && value.name === name; | ||
| } | ||
|
|
||
| export function getAstName(value: unknown): string | null { |
There was a problem hiding this comment.
getAstName is intentionally stricter than the old astName (which returned node.name whenever it was defined, regardless of node type). For the real Identifier/Literal/StringLiteral shapes the callers pass, this is equivalent. Just flagging the change so it's a conscious decision rather than an accidental tightening.
|
Review posted successfully. Here's my summary of the review for PR #1754: Review SummaryVerdict: Approve in spirit — clean, low-risk refactor. The PR extracts duplicated AST helper utilities into a shared
I traced each subtle semantic change between the old local helpers and the new shared ones, confirming behavioral equivalence (or slight improvement) for the real AST shapes from Vite's
Posted 2 optional nits (suggesting clarifying comments on the |
|
@james-elicx time for a 4th shard? |
yeah i was thinking about that as well, lets do it |
on it |
2 participants
Extracts small AST syntax helpers shared by AST-heavy plugin transforms.
PR #1736 has duplicated logic for loose AST node handling, child traversal, binding-pattern extraction, and Identifier/Literal name extraction. This PR lands the small shared utility first so #1736 can rebase onto it without carrying those local copies.
Validation: