fix(inertia): normalize empty props in PageProps - #2103
Conversation
🦋 Changeset detectedLatest commit: 3ae670e 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2103 +/- ##
=======================================
Coverage 92.25% 92.25%
=======================================
Files 116 116
Lines 4132 4132
Branches 1081 1080 -1
=======================================
Hits 3812 3812
Misses 285 285
Partials 35 35
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@yusukebe @ashunar0 |
|
After some thought, I've come to believe that the Union pattern is superior. I've included the implementation and comparison below. If you prefer this one, I'll make the revisions again. Implementationtype NormalizeProps<T> = T extends Record<string, never> ? {} : TComparisonconst _app = new Hono()
.use(inertia())
.get('/without-props', (c) => c.render('WithoutProps'))
.get('/optional-props',
(c) => c.render('OptionalProps'),
(c) => c.render('OptionalProps', { errors: { email: 'invalid' } })
)
|
| Pattern | Resolved type |
|---|---|
| Before the fix | Record<string, never> |
| Partial / Union | {} |
Accessing a prop that does not exist:
// Before the fix: no error — the index signature of Record<string, never> resolves it to never
props.foo // compiles silently
// Partial / Union: compile error
props.foo // TS2339: Property 'foo' does not exist on type '{}'OptionalProps (two handlers, with and without props)
| Pattern | Resolved type |
|---|---|
| Before the fix | { errors: { email: string } } | Record<string, never> |
| Partial | { errors?: { email: string } } |
| Union | { errors: { email: string } } | {} |
Accessing errors:
// Before the fix: type-checks as always present, but undefined at runtime
props.errors.email // compiles, crashes when rendered without props
// Partial: direct access, handle undefined normally
props.errors?.email
// Union: narrow first, then access
'errors' in props && props.errors.emailNote: a prop that does not exist (e.g. props.foo) is already a TS2339 error in all three versions — the { errors } member blocks it even before the fix.
Discriminated union style usage
If you expect to use the props like a discriminated union, the Union pattern is the more correct choice.
With the Partial pattern, the same code cannot guarantee props.error — it remains string | undefined.
const _app = new Hono()
.use(inertia())
.get(
'/union-props',
(c) => c.render('UnionProps'),
(c) => c.render('UnionProps', { kind: 'ok' as const, value: 0 }),
(c) => c.render('UnionProps', { kind: 'ng' as const, error: 'message' })
)// When `props.kind` is 'ng', `props.error` is guaranteed to be a string
if ('kind' in props && props.kind === 'ng') { props.error }By the way
I used the errors prop as an example, but since in Inertia it's generally common to obtain it from Form or useForm, this may not have been a very appropriate example. https://inertiajs.com/docs/v3/the-basics/forms#slot-props
|
@nkfr26 Sorry for the slow review. I pulled the branch locally and checked how the types actually behave in a few cases. First, thanks for catching these two problems. The second one in particular — Some context firstThere is no counterpart to this in inertia-laravel. Laravel has no mechanism to derive prop types from the server; you hand-write So "match the original" isn't available to us here. We have to decide what to prioritize ourselves. My two criteria:
Your concern about
|
| Before | Partial | Union | Proposal | |
|---|---|---|---|---|
| Nonexistent prop is an error | ❌ | ✅ | ✅ | ✅ |
props.errors?.email |
type lies | ✅ | ❌ | ✅ |
function Page({ errors }) |
✅ | ✅ | ❌ | ✅ |
| With 3 props, checking one settles the rest | — | ❌ | ✅ | ✅ |
error: string after kind === 'ng' |
— | ❌ | ✅ | ✅ |
I also verified:
- Page rendered without props only →
{}(sinceAllKeysresolves tonever), which is what this PR intends. - Props containing
defer→{ items: number[] } | { items?: never }. Normalization runs after prop resolution, so there's no interference. - Existing unions with no empty member (
{a} | {b}) → the condition doesn't match, so behavior is unchanged.
The trade-off
There's one way this loses to Partial: the type shown in the editor gets longer.
// Partial
{ errors?: { email: string } }
// Proposal
{ errors: { email: string } } | { errors?: never }?: never isn't self-explanatory on first sight, so the implementation should carry a comment. But that's a one-time cost to understand, whereas the ?. noise is something users pay on every line. I'd rather prioritize how the code reads over how the type displays.
Other notes
- Extracting
MethodOutput, introducingSimplify, and distributing overC extends unknownall look good to me. The distribution in particular is the right move — it makes barePagePropsnormalize per page. - Very minor:
.changeset/fast-kings-reply.mdis missing a trailing newline.
Summary
I'm on board with the direction. For the implementation I'd like to push for the optional never approach above rather than Partial or the union — it keeps both the ergonomics and the accuracy, and it leaves the existing union behavior alone.
|
@ashunar0 |
What
Normalize the props resolved by
PageProps:{}.never, so absent props must be accessed with?..Problem
Record<string, never>: accessing nonexistent props produces no error.Record<string, never> | { errors: Record<string, string> }. BecauseRecord<string, never>has an index signature,props.errorstype-checks as always present, but the prop can beundefinedat runtime when the props-less handler runs.Fix
Apply an internal
NormalizePropshelper insrc/page-props.ts:{}{ [K in keyof OtherVariants]?: never }, keeping existing property accesses valid while forcing?.for values that may be absentThe normalized result is flattened with
Simplify.The helper is not exported, so the public d.ts surface is unchanged.
Changes
src/page-props.ts— normalization,Simplifyflattening,MethodOutputdistributionsrc/page-props.test.ts— type tests viaPageProps+AppRegistry.changeset/fast-kings-reply.md— patch changesetType-only change.
The author should do the following, if applicable
pnpm changesetat the top of this repo and push the changeset