* Modify the Taskfiles to allow control of frontend package manager u…#5126
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughTaskfile templates and generation now support selecting a frontend package manager (npm, yarn, pnpm, bun) via ChangesFrontend Taskfile changes
Common template vars & Taskfile quoting
Template engine & build-assets Go changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
|
Hmm, Now I get to review the full changes, I can see that my editor has attempted to auto cleanup some stuff it possibly shouldn't have. I'll check this in the morning whilst testing on Windows and Linux |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
v3/internal/commands/build_assets/Taskfile.tmpl.yml (1)
12-13: ValidatePACKAGE_MANAGERbefore dispatching.These dispatchers build task names dynamically from
PACKAGE_MANAGER. Addingrequires.vars.enumhere would make unsupported values fail fast with a clear validation error instead of falling through to dynamic task lookup. Task supports enum validation specifically for string vars. (taskfile.dev)🧭 Suggested guard
install:frontend:deps: summary: Install frontend dependencies + requires: + vars: + - name: PACKAGE_MANAGER + enum: [npm, yarn, pnpm, bun] cmds: - task: install:frontend:deps:{{.PACKAGE_MANAGER}}Apply the same guard to
frontend:runanddev:frontend.Also applies to: 92-94, 197-197
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@v3/internal/commands/build_assets/Taskfile.tmpl.yml` around lines 12 - 13, Validate PACKAGE_MANAGER in the Taskfile template by adding requires.vars.enum entries for the tasks that dynamically use it (the task entries that call install:frontend:deps:{{.PACKAGE_MANAGER}}, frontend:run:{{.PACKAGE_MANAGER}} and dev:frontend:{{.PACKAGE_MANAGER}}) so unsupported values fail fast; update the Taskfile.tmpl.yml task definitions around the dynamic dispatch points to include requires.vars.enum with the allowed string options (e.g., "npm", "yarn", "pnpm") and apply the same guard for the other occurrences referenced (lines around the install:frontend:deps, frontend:run and dev:frontend dispatchers).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/internal/commands/build_assets/Taskfile.tmpl.yml`:
- Around line 28-39: The install:frontend:deps:bun task only lists bun.lockb as
a source, so dependency changes in Bun's text lockfile (bun.lock) won't
retrigger installs; update the Taskfile.tmpl.yml task named
install:frontend:deps:bun to include both "bun.lock" and "bun.lockb" in the
sources array (in addition to package.json) so the freshness detection watches
either lockfile and re-runs bun install when either changes.
- Around line 54-65: The task install:frontend:deps:yarn currently lists
generates: node_modules which fails with Yarn PnP; update the task to remove or
replace generates and add a status check that succeeds if either the PnP runtime
file exists or node_modules exists (e.g. a shell status like "sh: test -f
.pnp.cjs || test -f .pnp.js || test -d node_modules") so Taskfile freshness is
correct for both Yarn v2+ PnP and classic linkers; keep the existing dir,
sources and cmds and only change the generates -> status portion in the
install:frontend:deps:yarn task.
In `@v3/internal/templates/_common/Taskfile.tmpl.yml`:
- Line 4: Replace the hardcoded APP_NAME value ("wails3app") with the template
variable that injects the generated project name so downstream tasks (wails3
update build-assets, server binary naming, and Docker tag defaults) use the
actual project name; update the APP_NAME entry in Taskfile.tmpl to reference the
project-name template placeholder used elsewhere (the same placeholder used for
binary/Docker naming) so generated projects get their correct name.
---
Nitpick comments:
In `@v3/internal/commands/build_assets/Taskfile.tmpl.yml`:
- Around line 12-13: Validate PACKAGE_MANAGER in the Taskfile template by adding
requires.vars.enum entries for the tasks that dynamically use it (the task
entries that call install:frontend:deps:{{.PACKAGE_MANAGER}},
frontend:run:{{.PACKAGE_MANAGER}} and dev:frontend:{{.PACKAGE_MANAGER}}) so
unsupported values fail fast; update the Taskfile.tmpl.yml task definitions
around the dynamic dispatch points to include requires.vars.enum with the
allowed string options (e.g., "npm", "yarn", "pnpm") and apply the same guard
for the other occurrences referenced (lines around the install:frontend:deps,
frontend:run and dev:frontend dispatchers).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 41df8685-585a-4e54-b34b-f887afddbdeb
📒 Files selected for processing (2)
v3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/templates/_common/Taskfile.tmpl.yml
52355af to
2178aaf
Compare
|
This PR now extends the template data to include |
2178aaf to
dd482b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/internal/commands/build_assets/Taskfile.tmpl.yml`:
- Line 326: The log line contains a duplicated echo token ("echo echo \"Building
for device: UDID={{.Opn}}.UDID{{.Cls}} SCHEME={{.Opn}}.SCHEME{{.Cls}}
PROJECT={{.Opn}}.PROJECT{{.Cls}}\""); remove the extra leading "echo" so the
command is a single echo that prints the template string—locate the template
line containing the duplicated echo and edit it to use one echo while keeping
the {{.Opn}} and {{.Cls}} placeholders intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c0e8cd2d-5e29-49e7-8678-5962b25a3f56
📒 Files selected for processing (4)
v3/internal/commands/build-assets.gov3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/templates/_common/Taskfile.tmpl.ymlv3/internal/templates/templates.go
…sed via `PACKAGE_MANAGER` option
* Enrich template data with `{{.Opn}}` and `{{.Cls}}` to make writing Taskfile templates more predictable
dd482b1 to
d93a6b9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@v3/internal/commands/build_assets/Taskfile.tmpl.yml`:
- Around line 12-13: The task dispatchers in Taskfile.tmpl.yml use
{{.PACKAGE_MANAGER}} without a fallback, causing unresolved task names for
projects whose root Taskfile doesn't define PACKAGE_MANAGER; update each
dispatcher occurrence (e.g., the dispatch strings used by
install:frontend:deps:{{.Opn}}.PACKAGE_MANAGER{{.Cls}},
frontend:run:{{.Opn}}.PACKAGE_MANAGER{{.Cls}}, and
frontend:dev:{{.Opn}}.PACKAGE_MANAGER{{.Cls}}) to include an inline default
fallback (| default "npm") so the generated dispatch becomes
install:frontend:deps:{{.Opn}}.PACKAGE_MANAGER | default "npm"{{.Cls}} (and
similarly for the other two dispatchers) to preserve backward compatibility.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50b138a1-8cea-426f-9c4d-84b385177af0
📒 Files selected for processing (5)
v3/UNRELEASED_CHANGELOG.mdv3/internal/commands/build-assets.gov3/internal/commands/build_assets/Taskfile.tmpl.ymlv3/internal/templates/_common/Taskfile.tmpl.ymlv3/internal/templates/templates.go
✅ Files skipped from review due to trivial changes (1)
- v3/UNRELEASED_CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
- v3/internal/commands/build-assets.go
|
✅ Triaged by Wails PR Reviewer This PR has been reviewed and accepted. Test sub-issues have been created for all platforms. Head Ref OID: This comment serves as a signature that this PR has been triaged. Future runs will skip this PR based on the headRefOid. |
|
CI failure root cause: All TypeScript template variants are failing with: The problem is in # Current (broken for -ts templates):
- wails3 generate bindings -f '{{.Opn}}.BUILD_FLAGS{{.Cls}}' -clean=true {{- if .Typescript}}-ts{{end}}
# ^^ trim dash eats the spaceFix — move the space inside the - - wails3 generate bindings -f '{{.Opn}}.BUILD_FLAGS{{.Cls}}' -clean=true {{- if .Typescript}}-ts{{end}}
+ - wails3 generate bindings -f '{{.Opn}}.BUILD_FLAGS{{.Cls}}' -clean=true{{if .Typescript}} -ts{{end}}This gives:
Non-TypeScript templates pass today because the Taliesin is an AI agent. CC @leaanthony |
The `{{- if .Typescript}}` trim-left dash strips the whitespace before
the action, so moving `-ts` outside the `{{if}}` block (and dropping the
trim dash) keeps `-clean=true` and ` -ts` separated. Without this,
TypeScript templates produced `-clean=true-ts`, which fails parsing.
# Conflicts: # v3/internal/commands/build-assets.go # v3/internal/commands/build_assets/Taskfile.tmpl.yml
See #5125 for more information
Description
Change to the root Taskfile adding an extra var option and the common Taskfile for frontend management tasks.
The defaults keep execution flow as is and if accepted, i will happily add a note to the documentation site on how to control
How Has This Been Tested?
I have tested new projects on MacOS with npm, pnpm, and bun. I haven't tested with yarn although the code should work fine. I can do some further testing on windows and linux in the morning and will update accordingly
Summary by CodeRabbit
New Features
Refactor