Repository navigation
feat: ✨ add zsh dialect support (rebuild wasm against mvdan/sh v3.13.1) - #136
Conversation
🦋 Changeset detectedLatest commit: 7cda7c5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpgrades ChangesZsh dialect support and wasm rebuild
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Test as test/zsh.spec.ts
participant Parser as parse()
participant Printer as print()
Test->>Parser: parse(ZSH_SNIPPET, LangZsh)
Parser-->>Test: parsed AST
Test->>Printer: print(AST, LangZsh)
Printer-->>Test: reproduced snippet
Test->>Parser: parse(ZSH_SNIPPET, LangBash)
Parser-->>Test: error /ternary operator missing/
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
size-limit report 📦
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Fixes valid zsh scripts erroring with "ternary operator missing ? before :". See un-ts#135. - go.mod: bump mvdan/sh v3.11.0 -> v3.13.1 (requires Go 1.25) - src/types.ts: LangVariant became a bit-flag enum in mvdan/sh v3.13; update all values and add LangZsh - main.go / processor/structs_easyjson.go: marshal via jwriter to drop the net/http pull-in that TinyGo cannot compile for the js/wasm target - vendors/wasm_exec.cjs: regenerate to match TinyGo 0.41.1; a stale runtime silently corrupts wasm linear memory - rebuild main.wasm (TinyGo 0.41.1 + Go 1.25) - add zsh regression test; update snapshots for backtick-wrapped error tokens BREAKING CHANGE: LangVariant numeric values changed to bit flags (1 << iota); code passing raw variant numbers (rather than the LangVariant constants) must update. Co-Authored-By: Claude Opus 4.8 <[email protected]>
7311757 to
97bc6f3
Compare
🔴 Why SonarCloud & Codacy are red
✅ The one hand-written finding is already fixed
🛠️ Suggested resolution (maintainer side)
🟢 For reference — the checks that matter all pass
|
Co-Authored-By: Claude Opus 4.8 <[email protected]>
97bc6f3 to
14ab712
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
vendors/wasm_exec.cjs (1)
32-32: 📐 Maintainability & Code Quality | 🔵 TrivialStatic-analysis findings here are on vendored generated code — exclude
vendors/**rather than hand-edit.SonarCloud's
global→globalThisand optional-chain suggestions target the regenerated TinyGowasm_execglue. Editing them would diverge from the upstream generator output and violate the "use the samewasm_execas the TinyGo build" requirement, risking runtime incompatibility. Prefer excludingvendors/**from SonarCloud/Codacy (e.g. asonar-project.propertieswithsonar.exclusions=vendors/**) so the gate reflects hand-written code only.Also applies to: 538-544
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@vendors/wasm_exec.cjs` at line 32, The reported `global`/optional-chain findings are in vendored generated TinyGo glue (`wasm_exec`), so do not modify `vendors/wasm_exec.cjs` or related generated symbols like the `global.fs` setup. Instead, update the SonarCloud/Codacy configuration to exclude `vendors/**` so only hand-written code is analyzed, keeping the upstream `wasm_exec` output unchanged and avoiding divergence from the TinyGo-generated runtime.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@vendors/wasm_exec.cjs`:
- Line 32: The reported `global`/optional-chain findings are in vendored
generated TinyGo glue (`wasm_exec`), so do not modify `vendors/wasm_exec.cjs` or
related generated symbols like the `global.fs` setup. Instead, update the
SonarCloud/Codacy configuration to exclude `vendors/**` so only hand-written
code is analyzed, keeping the upstream `wasm_exec` output unchanged and avoiding
divergence from the TinyGo-generated runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bc584878-d40c-4671-b938-2b0411cee3fc
⛔ Files ignored due to path filters (4)
go.sumis excluded by!**/*.summain.wasmis excluded by!**/*.wasmtest/__snapshots__/fixtures.spec.ts.snapis excluded by!**/*.snaptest/__snapshots__/processor.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (8)
.changeset/gh-135-add-zsh-support.mdgo.modmain.goprocessor/structs_easyjson.gosrc/types.tstest/basic.spec.tstest/zsh.spec.tsvendors/wasm_exec.cjs
💤 Files with no reviewable changes (1)
- processor/structs_easyjson.go
✅ Files skipped from review due to trivial changes (2)
- test/basic.spec.ts
- .changeset/gh-135-add-zsh-support.md
🚧 Files skipped from review as they are similar to previous changes (4)
- test/zsh.spec.ts
- main.go
- go.mod
- src/types.ts
Address review on un-ts#136: re-apply the local modifications that the raw TinyGo output dropped, keeping the file matching the project's convention. - restore the `// modified based on <tinygo>/targets/wasm_exec.js` header, with the link updated to the TinyGo v0.41.1 source used for the rebuild - drop the Node CLI-runner block at the end (a local modification), so the file ends at `})();` as before Co-Authored-By: Claude Opus 4.8 <[email protected]>
commit: |
There was a problem hiding this comment.
Pull request overview
This PR updates the project’s embedded mvdan/sh parser and WASM runtime to add Zsh dialect support (fixing valid zsh like ${0:A:h:h} failing to parse) and aligns TypeScript/public APIs and tests with the newer parser behavior.
Changes:
- Bump
mvdan/shto v3.13.1 (Go 1.25) and update exportedLangVariantconstants to the new bit-flag values, addingLangZsh. - Adjust WASM/Go JSON marshaling to avoid
net/http(TinyGo js/wasm compatibility) and regenerate/update the WASM runtime shim. - Add Zsh regression tests and update snapshots for the new error-token formatting.
Reviewed changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
vendors/wasm_exec.cjs |
Regenerated TinyGo WASM runtime shim; updates Node built-in imports and WASI/timing glue. |
main.go |
Switch JSON marshaling to jwriter to avoid TinyGo-incompatible net/http dependency path. |
processor/structs_easyjson.go |
Removes unused easyjson package reference from generated file to prevent TinyGo pulls. |
src/types.ts |
Updates LangVariant to bit flags and adds LangZsh. |
test/zsh.spec.ts |
Adds regression coverage for parsing/printing Zsh parameter-expansion modifiers. |
test/basic.spec.ts |
Updates inline snapshot to match new backtick-wrapped error token formatting. |
test/__snapshots__/processor.spec.ts.snap |
Snapshot updates for new error token formatting. |
test/__snapshots__/fixtures.spec.ts.snap |
Snapshot updates for new error token formatting. |
go.mod |
Sets Go version to 1.25.0 and bumps mvdan.cc/sh/v3 to v3.13.1. |
go.sum |
Updates dependency checksums for mvdan.cc/sh/v3 v3.13.1. |
.changeset/gh-135-add-zsh-support.md |
Records the feature + breaking LangVariant renumbering for release automation. |
Files not reviewed (1)
- processor/structs_easyjson.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address Copilot review on un-ts#136: the `LangVariant` type only exposes the named bit-flag constants, so remove the doc claims that implied the legacy `0` value is usable, and reword the `variant?` default note accordingly. Co-Authored-By: Claude Opus 4.8 <[email protected]>
Address Copilot review on un-ts#136: point the reference at the specific issue instead of the generic issues list. Co-Authored-By: Claude Opus 4.8 <[email protected]>
|
@JounQin, copilot made one erroneous comment which I replied to, but made two valid comments regarding minor jsdoc inconsistencies which I fixed. Do the changes look good to merge? The quality gate is failing due to the generated vendor file, but we can't control those specific issues (they could be ignored with |
|
@jjloneman lint still doesn't pass on CI. |
The `LangVariant` `1 << iota` values are intentional bit flags, so scope off `@typescript-eslint/no-magic-numbers` for src/types.ts instead of an inline disable on the `LangZsh` line. Co-Authored-By: Claude Opus 4.8 <[email protected]>
- switch the zsh parse assertion from the generic `.resolves.toBeTruthy()` to the more semantic `.resolves.toBeDefined()` - convert the leading comment to a starred block to satisfy @stylistic/js/multiline-comment-style Co-Authored-By: Claude Opus 4.8 <[email protected]>
@JounQin, ah you're right. It was complaining about magic numbers for the iota sequence of the I also fixed the multiline -> block comment issue and ran Would you mind running the CI workflows again? Thanks! |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 4 4
Lines 77 77
Branches 29 29
=========================================
Hits 77 77 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Per review on un-ts#136, prefer an in-file disable over editing the shared eslint config: - add a scoped `@typescript-eslint/no-magic-numbers` disable block (with a `--` reason) around `LangVariant` - revert the `src/types.ts` override in eslint.config.js Co-Authored-By: Claude Opus 4.8 <[email protected]>
|



Fixes #135
🐛 What this fixes
${0:A:h:h}errored withternary operator missing ? before :.main.wasmvendoredmvdan/[email protected], which predates zsh support (added upstream in v3.13.0) — so there was nozshdialect to parse against.ternary operator missing ? before :— vendoredmvdan/shversion has no zsh dialect #135.🧪 Live demo (before / after)
Both run
npm startin an isolated container (each build registers its ownglobalThis.Go, so they're kept in separate sandboxes):[email protected], errors on valid zsh:${0:A:h:h}):🛠️ Changes
go.mod— bumpmvdan/shv3.11.0 → v3.13.1 (requires Go 1.25).src/types.ts—LangVariantbecame a bit-flag enum in mvdan/sh v3.13; update all values and addLangZsh.main.go+processor/structs_easyjson.go— marshal viajwriterdirectly to drop thenet/httppull-in that TinyGo can't compile for js/wasm.vendors/wasm_exec.cjs— regenerate to match the TinyGo used for the rebuild (a stale runtime silently corrupts wasm memory).main.wasm— rebuilt with TinyGo 0.41.1 + Go 1.25.test/zsh.spec.ts; update snapshots for mvdan/sh v3.13's backtick-wrapped error tokens.LangVariantnumeric values changed from a sequential enum to bit flags (1 << iota) to match mvdan/sh v3.13:LangBash:0→1LangPOSIX:1→2LangMirBSDKorn:2→4LangBats:3→8LangZsh: (new) →16LangAuto:4→32LangVariantconstants is unaffected; only code passing raw variant numbers needs updating.📦 Versioning
minorchangeset (→0.6.0), since the breakingLangVariantrenumber lands in the minor slot for a pre-1.0 package.✅ Testing
yarn vitest run→ 4 files / 7 tests passing, including the 3 new zsh tests, zero memory-corruption errors.main.wasmrunning under Node on macOS.📝 Notes for reviewers
processor/structs_easyjson.gois generated byyarn easyjson— the removal of the rooteasyjsonimport will be undone by regeneration, so it may want a post-generation patch (details in [bug] 🐛 valid zsh errors withternary operator missing ? before :— vendoredmvdan/shversion has no zsh dialect #135).main.wasmwas rebuilt with TinyGo 0.41.1 in a Linux container (a macOS/arm64 TinyGo codegen issue produced a corrupt wasm);vendors/wasm_exec.cjsmust stay in lockstep with that TinyGo version.Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Change
LangVariantto bit-flag values, addedLangZsh, and changed the numeric values for existing variants—integrations using raw numeric values must be updated.Chores