Fix Workflow interface to use array types instead of single-element tuples#187
Open
sohumt123 wants to merge 1 commit into
Open
Fix Workflow interface to use array types instead of single-element tuples#187sohumt123 wants to merge 1 commit into
sohumt123 wants to merge 1 commit into
Conversation
…uples The exported `Workflow` interface declared `tags`, `arguments`, and `shells` as single-element tuple types (`[string]`, `[Argument]`, `[Shell]`) rather than arrays. A tuple type of length one only accepts exactly one element, so under `strict` type-checking any workflow with two or more tags, arguments, or shells fails to type-check against the published `warp-workflows` types -- including the repo's own documented examples (FORMAT.md's three-argument for-loop and two-tag list, and the README example's `shells: []`, which a one-tuple rejects for having too few elements). 108 of the 332 shipped specs have two or more arguments. Change the three fields to array types (`string[]`, `Argument[]`, `Shell[]`). This is a declaration-only change; runtime is unaffected because index.ts casts the yaml-loader output with `as Workflow`. dist/index.d.ts regenerates from this source with the corrected array types. Add a type-level regression test (build_ts/type-tests/workflow-types.test-d.ts) that constructs Workflow literals mirroring the documented multi-value and empty-shells examples, compiled via a new tsconfig.typetest.json through the `check-types` script. It fails to compile before this change and passes after. An `exclude` entry keeps the fixture out of the emitted dist bundle.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Discord username (optional)
N/A
Description of changes
This is a type-only fix to the hand-written TypeScript source that backs the published
warp-workflowsnpm package (build_ts/index.ts), plus a type-level regression test.Problem
The exported
Workflowinterface declares three of its fields as single-element tuple types instead of arrays:A tuple type of length one accepts exactly one element. Under the package's
strictcompiler settings this means any workflow with two or more tags, arguments, or shells fails to type-check against the shippeddist/index.d.ts— and so do the repo's own documented examples:for {{variable}} in {{sequence}}; do {{command}}; donecommand with three arguments, and a two-tag listtags: ["git", "GitHub"].shells: [], which a one-tuple rejects for having too few elements.specs/have two or more arguments.So the published types contradict both the documentation and the majority of the real specs. Downstream consumers of
warp-workflowswho build aWorkflowobject with more than one tag/argument/shell get spuriousTS2322errors.Fix
Change the three fields to array types:
These are the only tuple-typed fields in the interface set (
Argumenthas scalar fields only,Shellis a const enum,WorkflowSlugis a string alias), so no other declarations need touching.This is a declaration-only change. Runtime is unaffected:
index.tscasts theyaml-loaderoutput withas Workflow, anddist/index.d.tsis generated from this source (it regenerates with the corrected array types on the next build).I also added a small type-level regression test so this can't silently regress:
build_ts/type-tests/workflow-types.test-d.ts— constructs twoWorkflowliterals: one mirroring FORMAT.md's three-argument / two-tag / two-shell example, and one mirroring the README's single-tag /shells: []example.build_ts/tsconfig.typetest.json— extendstsconfig.jsonwithnoEmitand includes the fixture.check-typesnpm script (tsc -p tsconfig.typetest.json).excludeentry intsconfig.json(node_modules,dist,type-tests) so the fixture is never emitted intodistor the published bundle. I verified the emitteddist/is unchanged apart from the correctedindex.d.ts.I intentionally did not wire
check-typesinto thebuildscript so the publish pipeline stays untouched — happy to add it (or a CI step) as a follow-up if you'd prefer it gated.Testing
Using the repo's pinned toolchain (
typescript@4.5.5, frombuild_ts/package-lock.json):Baseline sanity on the existing sources (unchanged behavior):
The regression fixture before the fix (against the original tuple types):
The same command after the fix, and the full build:
dist/index.d.tsafter the build now reads:and
dist/contains onlyindex.js,index.d.ts,index.js.map, andwarp-workflows.js(the fixture is excluded).crate-ci/typospasses clean on the changed files.Notes
ci.yamljob only runstypos+cargo), so this fixture can't fail/pass in the PR checks — the TS build runs inpublish.yamlafter merge. The before/aftertscoutput above is the evidence.noUncheckedIndexedAccesswill see e.g.workflow.tags[0]change fromstringtostring | undefined. That is the correct typing for an array, but flagging it in case it surfaces anything downstream.