Skip to content

Conversation

@jumski
Copy link
Contributor

@jumski jumski commented Oct 9, 2025

Problem

TypeScript failed to automatically infer handler parameter types in .array() and .map() methods, forcing users to add manual type annotations:

// Required manual annotations (BAD)
.array({ slug: 'fetch_users' }, async ({ run }: { run: { userIds: string[] } }) => {
  return run.userIds.map((id: string) => ({ id, name: `User_${id}` }));
})
.map({ slug: 'add_timestamps', array: 'fetch_users' }, (user: { id: string; name: string }) => ({
  ...user,
  createdAt: new Date().toISOString(),
}))

Root Cause

Complex conditional types on handler parameters prevented TypeScript from inferring the THandler type parameter:

// ❌ Prevented inference
handler: ValidateHandlerReturn<THandler, readonly any[]>
handler: TFlowInput extends readonly (infer Item)[]
  ? THandler & ((item: Item, ...) => ...)
  : never

Solution

Moved all type constraints from parameter types to type parameter constraints:

// ✅ Enables inference
THandler extends (...) => readonly any[] | Promise<readonly any[]>
handler: THandler

THandler extends TFlowInput extends readonly (infer Item)[]
  ? (item: Item, ...) => Json | Promise<Json>
  : never
handler: THandler

Additional Fixes

Client exhaustiveness checks: Replaced verbose unused variable pattern with cleaner satisfies never:

// Before
const _exhaustivenessCheck: never = event;

// After
event satisfies never;

Results

Type inference works automatically - no manual annotations needed
All 111 DSL type tests pass
All package typechecks pass (dsl, core, client, edge-worker)
Cleaner exhaustiveness checks - no unused variable warnings

Type System Changes

  • .array(): Handler constraint moved to type parameter
  • .map() (root): Handler constraint moved to type parameter
  • .map() (dependent): Handler constraint moved to type parameter
  • Removed unused ValidateHandlerReturn utility type

These changes maintain the same runtime behavior and type safety while enabling proper type inference.

@changeset-bot
Copy link

changeset-bot bot commented Oct 9, 2025

⚠️ No Changeset found

Latest commit: f2a7dc5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@jumski jumski marked this pull request as ready for review October 9, 2025 14:28
Copy link
Contributor Author

jumski commented Oct 9, 2025


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • merge:queue - adds this PR to the back of the merge queue
  • hotfix:queue - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@nx-cloud
Copy link

nx-cloud bot commented Oct 9, 2025

View your CI Pipeline Execution ↗ for commit f2a7dc5

Command Status Duration Result
nx test:types:health dsl ✅ Succeeded 16s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-10 17:29:42 UTC

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting

Comment on lines 56 to 51
# Run tsc directly on the file without --project flag
# This allows TS2578 errors (unused @ts-expect-error) to be detected
OUTPUT=$(pnpm tsc --noEmit "$file" 2>&1)
TSC_EXIT=$?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Preserve tsconfig when running per-file tsc checks

Removing the temporary config and calling pnpm tsc --noEmit "$file" causes the compiler to run with default options instead of those from tsconfig.typecheck.json. That means path aliases, esModuleInterop, moduleResolution, and other required settings are ignored. Any test file that imports packages through aliases or relies on non-default options now fails the strict check even when the project-wide pass succeeds (for example Pkgs/core/__tests__/types/PgflowSqlClient.test-d.ts now reports Cannot find module '@pgflow/dsl'). This produces false failures and masks the real intent of the second pass (detecting unused @ts-expect-error). The command still needs to use the project configuration while checking files individually.

Useful? React with 👍 / 👎.

@jumski jumski force-pushed the fix-type-test-harness branch 2 times, most recently from 3d797ba to edc0b5e Compare October 9, 2025 17:19
finalValue: input.normalStep.doubledValueArray.length,
}));
}))
.step({ slug: 'undefined' }, () => undefined);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces a type error by adding a step that returns undefined, which violates the newly added RejectUndefined type constraint. According to the type definition, step handlers must return a value other than undefined (though null is permitted). To fix this, modify the handler to return a valid value such as null or an object.

Suggested change
.step({ slug: 'undefined' }, () => undefined);
.step({ slug: 'undefined' }, () => null);

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines 595 to 599
map<Slug extends string, THandler extends (...args: any[]) => any>(
opts: Simplify<{ slug: Slug extends keyof Steps ? never : Slug } & StepRuntimeOptions>,
handler: TFlowInput extends readonly (infer Item)[]
? THandler & ((item: Item, context: BaseContext & TContext) => Json | Promise<Json>)
? RejectUndefined<THandler>
: never
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current implementation of the map function overload loses type information for the handler parameters. While adding the RejectUndefined constraint is good, it should preserve the parameter types:

handler: TFlowInput extends readonly (infer Item)[]
  ? RejectUndefined<(item: Item, context: BaseContext & TContext) => Json | Promise<Json>>
  : never

This ensures both the parameter types are preserved and the return type is properly constrained, maintaining type safety throughout the map operation.

Suggested change
map<Slug extends string, THandler extends (...args: any[]) => any>(
opts: Simplify<{ slug: Slug extends keyof Steps ? never : Slug } & StepRuntimeOptions>,
handler: TFlowInput extends readonly (infer Item)[]
? THandler & ((item: Item, context: BaseContext & TContext) => Json | Promise<Json>)
? RejectUndefined<THandler>
: never
map<Slug extends string, THandler extends (...args: any[]) => any>(
opts: Simplify<{ slug: Slug extends keyof Steps ? never : Slug } & StepRuntimeOptions>,
handler: TFlowInput extends readonly (infer Item)[]
? RejectUndefined<(item: Item, context: BaseContext & TContext) => Json | Promise<Json>>
: never

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@jumski jumski force-pushed the fix-type-test-harness branch 2 times, most recently from 9ddceb6 to 25868a2 Compare October 10, 2025 08:27
@jumski jumski force-pushed the fix-type-test-harness branch 2 times, most recently from 53fdbe8 to 93999c5 Compare October 10, 2025 11:44
@jumski jumski changed the title refactor(typecheck): fix tsc type tests by removing --project - tsc runs in strict mode then Fix Type Test Validation Issues Oct 10, 2025
- **After TypeScript version upgrades**
- **After Vitest updates**
- **After modifying tsconfig files**
- **After changing typecheck-strict.sh script**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The documentation in this file references typecheck-strict.sh, but this script has been renamed to typecheck-ts2578.sh in this PR. The documentation should be updated to reference the new script name to maintain consistency and avoid confusion for developers following these instructions.

Suggested change
- **After changing typecheck-strict.sh script**
- **After changing typecheck-ts2578.sh script**

Spotted by Graphite Agent

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@jumski jumski changed the title Fix Type Test Validation Issues Fix Type Test Validation Oct 10, 2025
@jumski jumski force-pushed the fix-type-test-harness branch 2 times, most recently from 32f65e0 to af4e0d5 Compare October 10, 2025 15:31
@jumski jumski changed the title Fix Type Test Validation Fix: DSL Type Inference for .array() and .map() Methods Oct 10, 2025
…uns in strict mode then

- Replaces temporary tsconfig creation with direct tsc invocation on each file
- Enables detection of unused @ts-expect-error errors during strict type checks
- Improves script clarity and reliability by removing unnecessary config file handling
@github-actions
Copy link
Contributor

🔍 Preview Deployment: Website

Deployment successful!

🔗 Preview URL: https://pr-248.pgflow.pages.dev

📝 Details:

  • Branch: fix-type-test-harness
  • Commit: 527aab53e0ddacb0af43e9d69a8f9be1eef2c376
  • View Logs

_Last updated: _

@github-actions
Copy link
Contributor

🔍 Preview Deployment: Playground

Deployment successful!

🔗 Preview URL: https://pr-248--pgflow-demo.netlify.app

📝 Details:

  • Branch: fix-type-test-harness
  • Commit: 527aab53e0ddacb0af43e9d69a8f9be1eef2c376
  • View Logs

_Last updated: _

@graphite-app
Copy link
Contributor

graphite-app bot commented Oct 10, 2025

Merge activity

  • Oct 10, 6:17 PM UTC: jumski added this pull request to the Graphite merge queue.
  • Oct 10, 6:17 PM UTC: CI is running for this pull request on a draft pull request (#252) due to your merge queue CI optimization settings.
  • Oct 10, 6:18 PM UTC: Merged by the Graphite merge queue via draft PR: #252.

graphite-app bot pushed a commit that referenced this pull request Oct 10, 2025
## Problem

TypeScript failed to automatically infer handler parameter types in `.array()` and `.map()` methods, forcing users to add manual type annotations:

```
// Required manual annotations (BAD)
.array({ slug: 'fetch_users' }, async ({ run }: { run: { userIds: string[] } }) => {
  return run.userIds.map((id: string) => ({ id, name: `User_${id}` }));
})
.map({ slug: 'add_timestamps', array: 'fetch_users' }, (user: { id: string; name: string }) => ({
  ...user,
  createdAt: new Date().toISOString(),
}))
```

## Root Cause

Complex conditional types on handler **parameters** prevented TypeScript from inferring the `THandler` type parameter:

```
// ❌ Prevented inference
handler: ValidateHandlerReturn<THandler, readonly any[]>
handler: TFlowInput extends readonly (infer Item)[]
  ? THandler & ((item: Item, ...) => ...)
  : never
```

## Solution

Moved all type constraints from parameter types to **type parameter constraints**:

```
// ✅ Enables inference
THandler extends (...) => readonly any[] | Promise<readonly any[]>
handler: THandler

THandler extends TFlowInput extends readonly (infer Item)[]
  ? (item: Item, ...) => Json | Promise<Json>
  : never
handler: THandler
```

### Additional Fixes

**Client exhaustiveness checks**: Replaced verbose unused variable pattern with cleaner `satisfies never`:

```
// Before
const _exhaustivenessCheck: never = event;

// After
event satisfies never;
```

## Results

✅ **Type inference works automatically** - no manual annotations needed
✅ **All 111 DSL type tests pass**
✅ **All package typechecks pass** (dsl, core, client, edge-worker)
✅ **Cleaner exhaustiveness checks** - no unused variable warnings

## Type System Changes

- `.array()`: Handler constraint moved to type parameter
- `.map()` (root): Handler constraint moved to type parameter
- `.map()` (dependent): Handler constraint moved to type parameter
- Removed unused `ValidateHandlerReturn` utility type

These changes maintain the same runtime behavior and type safety while enabling proper type inference.
@graphite-app graphite-app bot closed this Oct 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants