Add types to the library #28
Conversation
- Added some simple basic types - Made actual progress on the `object` schema typing
Because we enabled `noImplicitAny` the `typeAcquisition` had to be removed. The reason is that this doesn't work because the 'acquired' types contain implicit any. Only adding separate type packages does not work because the aquired types shadow (partially) the installed types. It is quite a maze to get right...
WalkthroughAdds a comprehensive TypeScript type surface and many type-only tests, tightens JSDoc/type annotations across hooks/fields/snapshot/state, introduces a postinstall script to install peer deps into a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Root as root package.json
participant Postinstall as scripts/install-peer-dependencies-for-typescript.js
participant FS as filesystem (typescript/)
participant Yarn as yarn
Root->>Postinstall: run postinstall
Postinstall->>Postinstall: read peerDependencies
Postinstall->>FS: ensure `typescript/` dir and package.json
Postinstall->>Yarn: spawn `yarn add <peer@version> --pure-lockfile` (cwd=typescript/)
Yarn-->>Postinstall: stream logs and exit code
Postinstall->>Postinstall: exit with yarn code (PREVENT_LOOP=true)
sequenceDiagram
autonumber
participant Caller as subscribeToAll caller
participant subscribeToAll as src/state.subscribeToAll
participant Child as child subscription
Caller->>subscribeToAll: subscribe({ state, childrenFromState, notify, ... })
subscribeToAll->>Child: subscribeToChild(...) for each derived child
Child-->>subscribeToAll: child notifies
subscribeToAll->>Caller: notify(newState) %% oldState removed
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Pre-merge checks and finishing touches✅ Passed checks (3 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
EECOLOR
left a comment
EECOLOR
left a comment
There was a problem hiding this comment.
- Je moet nog testen of het goed werkt wanneer je de library in een project gebruikt
- Je moet nog een
test.typesscript toevoegen die de*.type.test.tsfile compiled
|
@buildersteven4 I can only add you as reviewer once you have accepted the invitation :-D |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/state.js (1)
52-61: Preserve the(newState, oldState)notification contract.
notifyused to receive bothnewStateandoldState, and callers still rely on that signature. Dropping the second argument leaves listeners witholdState === undefined, silently breaking diff-based consumers. Please keep passing both values through.- if (onlyNotifyOnChildChange && !childrenChanged) return - notify(newState) + if (onlyNotifyOnChildChange && !childrenChanged) return + notify(newState, oldState)src/fields.js (3)
55-57: Reset must clear object-level errors.Line 55 currently replays the old
errorbecauseupdateStatespreads the previous state before applying the pick. After a validation failure, callingreset()leavesstate.get().error(and thereforeinvalid) stuck at the stale value, so the form never returns to a clean state. Include the initialerrorin the reset payload so the state realigns with a fresh form.- internalState.update(x => - updateState(x, pick(initialState, ['isSubmitted'])) - ) + internalState.update(x => + updateState(x, pick(initialState, ['error', 'isSubmitted'])) + )
133-136: Array reset keeps stale validation errors.Because the
updateStatecall does not overrideerror, any failure returned byfield.validatepersists afterreset(), leavingstate.get().invalidtrue and the array field permanently marked invalid. Add the initialerroralongsidechildrenandisSubmittedwhen rebuilding the state.- const { children } = internalState.update(x => - updateState(x, pick(initialState, ['children', 'isSubmitted'])) - ) + const { children } = internalState.update(x => + updateState(x, pick(initialState, ['children', 'error', 'isSubmitted'])) + )
202-205: Basic field reset leaveserrorandinvalidlatched.After a validation failure, calling
reset()should return the field to a clean slate. The current implementation keeps the previouserror, sostate.get().invalidnever clears. Make sure we reset the error alongside value/flags.- internalState.update(x => - updateState(x, pick(initialFormFieldState, ['value', 'isSubmitted', 'isVisited'])) - ) + internalState.update(x => + updateState(x, pick(initialFormFieldState, ['error', 'value', 'isSubmitted', 'isVisited'])) + )
🧹 Nitpick comments (1)
src/fields.js (1)
75-75: Rename local to avoidconstructorshadowing.Biome flags this identifier because
constructoris a restricted global name; keeping it trips lint. Rename the local binding so the pipeline stays green.- const constructor = constructors[normalizedField.type] - return constructor({ + const buildField = constructors[normalizedField.type] + return buildField({
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (25)
.gitignore(1 hunks)example/tsconfig.json(1 hunks)package.json(1 hunks)scripts/install-peer-dependencies-for-typescript.js(1 hunks)scripts/tsconfig.json(1 hunks)src/components.js(1 hunks)src/components.type.test.ts(1 hunks)src/fields.js(9 hunks)src/fields.type.test.ts(1 hunks)src/hooks.js(7 hunks)src/hooks.type.test.ts(1 hunks)src/normalize.js(2 hunks)src/normalize.type.test.ts(1 hunks)src/schema.js(2 hunks)src/schema.type.test.ts(1 hunks)src/snapshot.js(4 hunks)src/snapshot.type.test.ts(1 hunks)src/state.js(4 hunks)src/type-helpers.js(1 hunks)src/type.test.helpers.ts(1 hunks)src/types.d.ts(0 hunks)src/types.ts(1 hunks)src/validation.js(1 hunks)tsconfig.json(1 hunks)types/global.d.ts(0 hunks)
💤 Files with no reviewable changes (2)
- types/global.d.ts
- src/types.d.ts
🧰 Additional context used
🧬 Code graph analysis (13)
src/components.type.test.ts (5)
src/hooks.js (2)
useForm(25-56)form(31-35)src/validation.js (1)
optionalT(22-24)src/schema.js (2)
object(19-21)array(39-41)src/components.js (4)
FormFieldValue(13-16)value(14-14)FormFieldsValues(24-27)FormFieldValid(35-38)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)
src/hooks.type.test.ts (6)
src/hooks.js (10)
useForm(25-56)form(31-35)useFormFieldValue(129-131)useFormFieldsValues(137-139)useFormField(145-151)useNumberFormField(156-168)useBooleanFormField(171-181)useObjectFormField(198-203)useArrayFormField(187-192)useFormFieldSnapshot(113-122)src/validation.js (1)
optionalT(22-24)src/schema.js (2)
object(19-21)array(39-41)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)src/types.ts (9)
Field(162-162)Basic(144-147)Basic(203-213)Basic(258-260)Basic(298-299)State(246-246)Event(215-219)Common(249-256)Snapshot(289-289)src/components.js (1)
useFormFieldSnapshot(36-36)
src/schema.type.test.ts (5)
src/type-helpers.js (1)
asConst(5-7)src/validation.js (8)
optional(50-50)optional(50-50)required(51-53)required(51-53)number(55-57)number(55-57)src/types.ts (2)
Validate(23-27)InitialValue(33-34)src/schema.js (2)
object(19-21)array(39-41)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectAssignable(15-15)Prepared(28-29)expectNotNever(2-2)
src/normalize.js (1)
src/hooks.js (3)
field(147-147)field(188-188)field(199-199)
src/fields.type.test.ts (6)
src/type-helpers.js (1)
asConst(5-7)src/normalize.js (1)
normalize(9-17)src/schema.js (2)
object(19-21)array(39-41)src/fields.js (7)
validate(28-28)validate(102-102)validate(180-180)createObjectFormField(21-83)value(30-40)value(104-118)value(182-190)src/types.ts (9)
ValidationContext(31-31)State(246-246)Readonly(268-271)Basic(144-147)Basic(203-213)Basic(258-260)Basic(298-299)Field(162-162)Event(215-219)src/type.test.helpers.ts (4)
expectAssignable(15-15)Prepared(28-29)expectNotAny(12-12)expectNotNever(2-2)
src/normalize.type.test.ts (5)
src/type-helpers.js (1)
asConst(5-7)src/schema.js (2)
object(19-21)array(39-41)src/types.ts (1)
ValidationFunction(28-28)src/normalize.js (1)
normalize(9-17)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)
src/components.js (2)
src/hooks.js (6)
useFormFieldValue(129-131)field(147-147)field(188-188)field(199-199)useFormFieldsValues(137-139)useFormFieldSnapshot(113-122)src/snapshot.js (3)
field(49-49)field(75-75)field(100-100)
src/types.ts (2)
src/components.js (1)
value(14-14)src/fields.js (3)
value(30-40)value(104-118)value(182-190)
src/fields.js (2)
src/state.js (3)
subscribeToChildren(79-91)createState(8-28)state(9-9)src/normalize.js (1)
normalize(9-17)
src/schema.js (1)
src/fields.js (1)
fields(23-23)
src/snapshot.type.test.ts (6)
src/hooks.js (2)
useForm(25-56)form(31-35)src/validation.js (2)
optionalT(22-24)requiredT(46-48)src/schema.js (2)
object(19-21)array(39-41)src/type-helpers.js (1)
asConst(5-7)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)src/types.ts (2)
Falsy(15-15)ValidationError(30-30)
src/snapshot.js (1)
src/state.js (1)
subscribeToChildren(79-91)
src/hooks.js (2)
example/src/domain/Full.js (5)
useForm(58-64)field(207-207)field(215-215)field(223-223)fields(12-48)src/type-helpers.js (1)
asAny(10-12)
🪛 Biome (2.1.2)
src/components.type.test.ts
[error] 7-7: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/hooks.type.test.ts
[error] 17-17: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 57-57: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 60-60: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 63-63: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 77-77: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 91-91: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 105-105: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 119-119: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 144-144: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/fields.js
[error] 74-74: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/snapshot.type.test.ts
[error] 9-9: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/hooks.js
[error] 10-10: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
|
Note to self: this is a major version update, see the code rabbit comment about state ( |
|
@coderabbitai about the out-of-range comments concerning |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1282 🏁 Script executed: Length of output: 1177 🏁 Script executed: Length of output: 1067 @EECOLOR You're absolutely right! The subscription on line 37 ensures that validation is automatically triggered whenever the form value changes, including when Here's the flow:
This is a clean reactive design pattern where validation is consistently applied to all value changes regardless of the source (user input, programmatic updates, or reset). The subscription established during form initialization ensures validation stays in sync with the form state. Thank you for the clarification! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
package.json (1)
28-31: Good conditional postinstall implementation!The postinstall script now correctly handles the case where the script file doesn't exist, preventing failures in consumer installs. The
test.typesscript properly references the new type-test configuration.One minor observation: the trailing semicolon after
fion line 29 is unnecessary (though harmless). Consider:- "postinstall": "if test -f ./scripts/install-peer-dependencies-for-typescript.js; then node ./scripts/install-peer-dependencies-for-typescript.js; fi;", + "postinstall": "if test -f ./scripts/install-peer-dependencies-for-typescript.js; then node ./scripts/install-peer-dependencies-for-typescript.js; fi",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
package.json(1 hunks)src/fields.js(9 hunks)src/fields.type.test.ts(1 hunks)src/hooks.js(7 hunks)src/types.ts(1 hunks)tsconfig.type.test.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/fields.js
🧰 Additional context used
🧬 Code graph analysis (3)
src/fields.type.test.ts (6)
src/type-helpers.js (1)
asConst(5-7)src/normalize.js (1)
normalize(9-17)src/schema.js (2)
object(19-21)array(39-41)src/fields.js (7)
validate(28-28)validate(102-102)validate(180-180)createObjectFormField(21-83)value(30-40)value(104-118)value(182-190)src/types.ts (9)
ValidationContext(31-31)State(246-246)Readonly(268-271)Basic(144-147)Basic(203-213)Basic(258-260)Basic(298-299)Field(162-162)Event(215-219)src/type.test.helpers.ts (4)
expectAssignable(15-15)Prepared(28-29)expectNotAny(12-12)expectNotNever(2-2)
src/types.ts (2)
src/fields.js (3)
value(30-40)value(104-118)value(182-190)src/components.js (1)
value(14-14)
src/hooks.js (1)
src/type-helpers.js (1)
asAny(10-12)
🪛 Biome (2.1.2)
src/hooks.js
[error] 10-10: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 25-25: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (13)
tsconfig.type.test.json (1)
1-6: LGTM!The TypeScript configuration correctly extends the base config and appropriately narrows the scope to type-test files. This setup aligns well with the new
test.typesscript in package.json.package.json (1)
32-42: LGTM!The peer dependencies and dev dependencies are appropriate for adding TypeScript support:
- React 18.3.1 aligns with modern React usage
- TypeScript 5.8.2 and corresponding @types packages are recent and compatible
- The setup supports both runtime (peerDependencies) and development-time (devDependencies) typing needs
src/fields.type.test.ts (1)
1-135: LGTM!The type-level tests are comprehensive and correctly validate:
- Basic field types with different validation configurations (optional, single validation, multiple validations)
- Object field composition and value aggregation
- Nested structures with both object and array children (correctly including both in the expected value type at line 127)
- Event handler signatures and state management types
The tests provide strong type-safety guarantees for the form field system.
src/hooks.js (6)
12-56: LGTM!The
useFormhook now has comprehensive TypeScript generics and JSDoc annotations. Key improvements:
- Proper template parameters for fields, initial values, and validation
- Correct type reference
Field.ObjectFromObjectInput<A>on line 27 (previous typo resolved)- The form initialization and subscription setup on lines 29-39 correctly triggers validation on value changes
Note: The static analysis warning about line 25 is a false positive—
useFormis the hook declaration itself, not a conditional hook call.
58-106: LGTM!The state management hooks are well-typed:
useFormFieldStateproperly extracts the inner value type fromState.Readonly<T>useFieldStatescorrectly handles tuple typing withStateTupleToValueTuplecast on line 104- The dependency array on line 98 appropriately passes the states array directly, as explained in the comment block at lines 205-214
108-131: LGTM!The snapshot and value hooks are properly typed:
useFormFieldSnapshotconstructs a memoized state object with@satisfies State.Readonlyannotation ensuring type correctnessuseFormFieldValuecleanly delegates touseFormFieldState- Return types are accurately derived from the field types
133-139: LGTM!The
useFormFieldsValueshook correctly maps field values with proper tuple typing via theMapTuple<T, 'value'>cast on line 138, preserving tuple structure and element types.
141-181: LGTM!The form field hooks are well-implemented with proper type annotations:
useFormFieldprovides clean extraction of field propertiesuseNumberFormFieldcorrectly handles number conversion, including edge cases (empty string, NaN)useBooleanFormFieldproperly extracts thecheckedproperty- Event handlers are correctly typed with
React.ChangeEvent<HTMLInputElement>
183-203: LGTM!The composite field hooks are properly implemented:
useArrayFormFieldreturnshelpers(add/remove operations) appropriate for array manipulationuseObjectFormFieldreturnsfields(child form fields) appropriate for object structure- Both hooks are correctly typed with their respective generics (
Field.ArrayandField.Object)The semantic distinction between returning
helpersfor arrays andfieldsfor objects aligns well with their different usage patterns.src/types.ts (4)
1-34: LGTM!The utility types are well-designed and serve clear purposes:
Expandimproves IDE type hint readability (lines 5-6)PartialWithStringKeyhandles TypeScript's keyof string constraints (lines 12-13)MapTupleprovides tuple-preserving mapping (lines 18-21)- Validation types (
Validate,ValidationFunction,ValidationError,ValidationContext) comprehensively model the validation pipeline
36-92: LGTM!The
FieldInputandFieldSchemanamespaces provide robust type mappings:
FieldInputproperly bridges toFieldSchemavalues (lines 38-42)FieldSchema.ToValuehandles all schema variants through conditional types (lines 58-63)- Object and Array schema types are well-defined with proper field and validation typing
ExtractObjectFieldscorrectly handles both direct object fields and constructor patterns (lines 71-74)
94-244: LGTM!The
NormalizedFieldandFieldnamespaces provide comprehensive runtime field modeling:
NormalizedField.FromFieldSchemacorrectly transforms all schema variants (lines 103-136)ValidateToValidationFunctionproperly handles null/undefined/Validate casesFieldtypes accurately model runtime structure with state, event handlers, and helpers- Line 174 correctly uses
ObjectFromObjectInput(previous typo concern resolved)BaseFieldPropertiesis well-extracted for reuse across field types (lines 195-201)- Event types properly model both direct values and event objects (lines 215-219)
246-326: LGTM!The
StateandSnapshotnamespaces complete the type system:
State.Commondefines shared state properties (error, validity, focus, etc.) at lines 249-256- Clear distinction between
Readonly(single value subscription) andReadonlyWithHistory(with old value) at lines 268-275Subscriptiontypes properly handle both variants (lines 279-280)StateTupleToValueTupleenables type-preserving tuple mapping (lines 283-286)Snapshottypes correctly model immutable submission data with nested value/error structureMapToValuesandMapToErrorsprovide comprehensive field-to-value/error mapping (lines 321-325)
- More precise typing for error and related booleans - Partial is now recursive - Additional utility type to more easily get the form shape from fields - Export new validation functions - Export types
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
example/src/domain/Full.js (2)
58-85: Don’t discard the submitted form data.Casting
snapshot.valueto a boolean means we throw away the actual submission payload, so the thank-you view just renderstrueinstead of the form contents. The example relied on showing the captured values. Keep storing the snapshot value (and type it accordingly) rather than collapsing it to a flag; the existingsubmitted ? … : …guard still works because any non-null object is truthy.- const [submitted, setSubmitted] = React.useState(/** @type {boolean | null} */ (null)) + const [submitted, setSubmitted] = React.useState( + /** @type {Snapshot.FromObjectFields<typeof fields>['value'] | null} */ (null) + ) … - setSubmitted(Boolean(snapshot.value)) + setSubmitted(snapshot.value)Remember to update
Bedankt’s prop type to expect the same value shape.
168-176: AlignBedanktwith the restored submission value.Once
submittedholds the actual snapshot data again, this prop typing must reflect that; otherwise we’re still treating it as a boolean and the example output stays wrong.-/** @arg {{ submitted: boolean, onReset: () => void }} props */ +/** @arg {{ submitted: Snapshot.FromObjectFields<typeof fields>['value'], onReset: () => void }} props */ function Bedankt({ submitted, onReset }) { … - <Code value={submitted} indent /> + <Code value={submitted} indent />example/src/domain/Basic.js (1)
41-54: Fix typo in error message.Line 51 contains a typo: 'unkown error' should be 'unknown error'.
Apply this diff to fix the typo:
- {showError && <p>{hasKey(validationErrors, error.id) ? validationErrors[error.id] : 'unkown error'}</p>} + {showError && <p>{hasKey(validationErrors, error.id) ? validationErrors[error.id] : 'unknown error'}</p>}
♻️ Duplicate comments (3)
tsconfig.json (1)
24-26: Restore module resolution for peer types.Mapping
"*"solely to"./typescript/*"still prevents the compiler from findingreact,@types/react, etc.—those packages are installed intypescript/node_modules, not attypescript/<name>, so every bare import resolves to a non-existent path. Please reinstate the node_modules entries (per the earlier feedback) so TypeScript can see the installed peers.- "paths": { - "*": ["./typescript/*"] - } + "paths": { + "*": [ + "./typescript/node_modules/*", + "./typescript/*", + "*" + ] + }package.json (1)
28-31: Fix the postinstall guard — it still breaks installs.
if test -f …is POSIX-only and returns exit code 1 when the script is absent, so consumers on Unix still get a failing postinstall, and Windows installs explode outright becausetest/that shell syntax doesn’t exist. We need a cross-platform check that always exits 0 when the helper script isn’t present (and ideally ship the script, or guard inside it). One workable option:- "postinstall": "if test -f ./scripts/install-peer-dependencies-for-typescript.js; then node ./scripts/install-peer-dependencies-for-typescript.js; fi", + "postinstall": "node -e \"const fs=require('fs'); const path=require('path'); const { execFileSync } = require('child_process'); const script=path.join(__dirname,'scripts','install-peer-dependencies-for-typescript.js'); if (!fs.existsSync(script)) process.exit(0); execFileSync(process.execPath, [script], { stdio: 'inherit' });\"",(Alternatively, include
scripts/install-peer-dependencies-for-typescript.jsin thefilesarray and perform the existence check at the top of that script.)src/hooks.js (1)
26-26: Fix typo in Field type alias.
Field.ObjecFromObjectInputis misspelled (missing the 't'). This should beField.ObjectFromObjectInputto match the type defined in src/types.ts.Apply this diff to fix the typo:
- const formRef = React.useRef(/** @type {Field.ObjecFromObjectInput<A>} */ (asAny(null))) + const formRef = React.useRef(/** @type {Field.ObjectFromObjectInput<A>} */ (asAny(null)))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
example-firebase/yarn.lockis excluded by!**/yarn.lock,!**/*.lockexample/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (55)
example-firebase/.editorconfig(0 hunks)example-firebase/.eslintrc(0 hunks)example-firebase/.gitignore(0 hunks)example-firebase/.stylelintrc(0 hunks)example-firebase/bin/postinstall(0 hunks)example-firebase/config/default.js(0 hunks)example-firebase/config/dev.js(0 hunks)example-firebase/config/firebase-database-rules.js(0 hunks)example-firebase/config/firebase-storage.rules(0 hunks)example-firebase/package.json(0 hunks)example-firebase/scripts/deploy-database-rules.js(0 hunks)example-firebase/scripts/deploy-storage-rules.js(0 hunks)example-firebase/scripts/machinery/run.js(0 hunks)example-firebase/scripts/readme.md(0 hunks)example-firebase/services/form-processing-service.js(0 hunks)example-firebase/services/machinery/createFirebaseServiceApp.js(0 hunks)example-firebase/services/machinery/sendEmail.js(0 hunks)example-firebase/services/machinery/serviceApplication.js(0 hunks)example-firebase/services/readme.md(0 hunks)example-firebase/src/cssGlobal/color.css(0 hunks)example-firebase/src/cssGlobal/media.css(0 hunks)example-firebase/src/cssGlobal/type.css(0 hunks)example-firebase/src/domain/App.js(0 hunks)example-firebase/src/domain/Firebase.js(0 hunks)example-firebase/src/index.css(0 hunks)example-firebase/src/index.html.js(0 hunks)example-firebase/src/machinery/readme.md(0 hunks)example-firebase/src/reset.css(0 hunks)example-firebase/src/robots.txt(0 hunks)example-firebase/tsconfig.json(0 hunks)example-firebase/types/global.d.ts(0 hunks)example/.gitignore(1 hunks)example/README.md(1 hunks)example/bin/postinstall(0 hunks)example/config/default.js(1 hunks)example/package.json(1 hunks)example/src/cssGlobal/color.css(0 hunks)example/src/cssGlobal/media.css(0 hunks)example/src/cssGlobal/type.css(0 hunks)example/src/domain/Basic.js(4 hunks)example/src/domain/Full.js(7 hunks)example/src/domain/machinery/Form.js(4 hunks)example/src/domain/machinery/validation.js(1 hunks)example/src/index.css(1 hunks)example/tsconfig.json(1 hunks)package.json(1 hunks)src/fields.js(8 hunks)src/fields.type.test.ts(1 hunks)src/hooks.js(7 hunks)src/normalize.js(2 hunks)src/types.ts(1 hunks)src/validation.js(1 hunks)tsconfig.json(1 hunks)types.ts(1 hunks)validation.js(1 hunks)
💤 Files with no reviewable changes (35)
- example-firebase/src/cssGlobal/type.css
- example-firebase/src/reset.css
- example-firebase/scripts/machinery/run.js
- example-firebase/services/readme.md
- example-firebase/src/machinery/readme.md
- example-firebase/services/machinery/serviceApplication.js
- example-firebase/src/index.html.js
- example-firebase/types/global.d.ts
- example/src/cssGlobal/type.css
- example-firebase/src/cssGlobal/media.css
- example-firebase/.gitignore
- example-firebase/.editorconfig
- example-firebase/bin/postinstall
- example-firebase/package.json
- example-firebase/scripts/deploy-storage-rules.js
- example-firebase/src/index.css
- example-firebase/services/machinery/sendEmail.js
- example/src/cssGlobal/media.css
- example-firebase/config/dev.js
- example-firebase/config/default.js
- example-firebase/src/domain/Firebase.js
- example-firebase/.stylelintrc
- example-firebase/config/firebase-database-rules.js
- example-firebase/.eslintrc
- example/src/cssGlobal/color.css
- example-firebase/src/cssGlobal/color.css
- example-firebase/scripts/deploy-database-rules.js
- example-firebase/src/robots.txt
- example-firebase/services/form-processing-service.js
- example-firebase/scripts/readme.md
- example-firebase/services/machinery/createFirebaseServiceApp.js
- example-firebase/tsconfig.json
- example-firebase/config/firebase-storage.rules
- example/bin/postinstall
- example-firebase/src/domain/App.js
✅ Files skipped from review due to trivial changes (4)
- example/.gitignore
- example/README.md
- src/fields.type.test.ts
- example/src/domain/machinery/Form.js
🚧 Files skipped from review as they are similar to previous changes (2)
- example/tsconfig.json
- src/validation.js
🧰 Additional context used
🧬 Code graph analysis (6)
example/src/domain/Basic.js (1)
src/validation.js (2)
requiredT(52-54)error(99-99)
src/hooks.js (2)
src/type-helpers.js (1)
asAny(10-12)src/fields.js (4)
fields(23-23)value(30-40)value(104-118)value(182-190)
example/src/domain/machinery/validation.js (2)
src/validation.js (1)
error(99-99)src/hooks.js (1)
form(30-34)
src/fields.js (2)
src/state.js (3)
subscribeToChildren(79-91)createState(8-28)state(9-9)src/normalize.js (1)
normalize(9-17)
example/src/domain/Full.js (2)
src/type-helpers.js (1)
asConst(5-7)src/validation.js (5)
optionalT(22-24)required(57-59)required(57-59)error(99-99)requiredT(52-54)
src/types.ts (1)
src/fields.js (3)
value(30-40)value(104-118)value(182-190)
🪛 Biome (2.1.2)
src/hooks.js
[error] 10-10: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (24)
example/src/index.css (1)
13-16: Typography tokens look good.Using literal values keeps the previous defaults intact, so the example’s baseline styling remains consistent.
types.ts (1)
1-1: LGTM!Clean barrel export pattern for the public types API.
validation.js (1)
1-7: LGTM!The addition of
requiredTandoptionalTexports expands the validation API surface while maintaining backward compatibility.example/src/domain/Basic.js (3)
1-23: LGTM!The updated imports and field definitions properly integrate with the new type system. The use of
requiredT('string')enables type-safe validation for the name field.
34-38: LGTM!The JSDoc annotation properly documents the snapshot parameter.
56-59: LGTM!The
hasKeytype guard helper is correctly implemented with a proper type predicate, enabling type-safe access to validation error messages.example/src/domain/machinery/validation.js (2)
1-20: LGTM!The type annotations for
ifFormHasValueproperly constrain the function with template parameters while preserving runtime behavior.
22-36: LGTM!The type annotations for
ifParentHasValuecorrectly document the template-based validation function composition pattern.src/normalize.js (3)
1-17: LGTM!The generic template parameter and type cast enable proper type inference for normalized fields while maintaining runtime behavior.
19-66: LGTM!The type annotations for all conversion helper functions properly document their input/output contracts and return types.
68-105: LGTM!The enhanced
toValidationFunctionproperly handles the validation function composition with explicit null handling and type annotations.src/fields.js (4)
1-83: LGTM!The
createObjectFormFieldfunction is comprehensively typed with template parameters. The@satisfiesannotation on the value getter (lines 30-40) ensures type safety while the runtime behavior remains unchanged.
85-166: LGTM!The
createArrayFormFieldfunction maintains runtime behavior while adding comprehensive type annotations. TheState.Readonlywrapper on lines 104-118 properly types the array value state.
168-224: LGTM!The
createBasicFormFieldimplementation properly usesState.ReadonlyWithHistory(lines 182-190) to type the value state, preserving the history-aware subscription semantics.
226-321: LGTM!The helper functions (
updateState,pick,deriveFormFieldState,mapValues,bindValidate,addParent) are properly typed with comprehensive JSDoc annotations. Thets-expect-errorcomments appropriately mark locations where TypeScript's inference falls short for complex reduce operations.src/hooks.js (3)
12-55: LGTM!The
useFormhook is properly typed with template parameters for fields and validation. The implementation correctly maintains form state and validation subscriptions (line 36).
57-139: LGTM!The state and value hooks (
useFormFieldState,useFieldStates,useFormFieldSnapshot,useFormFieldValue,useFormFieldsValues) are properly typed with comprehensive template parameters and return types.
141-202: LGTM!The specialized field hooks (
useFormField,useNumberFormField,useBooleanFormField,useArrayFormField,useObjectFormField) properly implement type-safe event handling and state management.src/types.ts (6)
1-47: LGTM!The foundational type utilities (
Expand,PartialWithStringKey,Falsy,MapTuple) and validation types (Validate,ValidationFunction,ValidationContext) provide a solid base for the type system. TheInitialValuetype properly handles partial nested structures.
48-104: LGTM!The
FieldInputandFieldSchemanamespaces properly model the input and normalized schema representations with appropriate value extraction utilities.
106-172: LGTM!The
NormalizedFieldnamespace correctly models normalized field structures with proper validation function typing and value extraction.
174-253: LGTM!The
Fieldnamespace comprehensively models runtime field instances with proper type mappings. Note thatObjectFromObjectInputon line 186 is correctly spelled (with the 't'), but its usage in src/hooks.js still has the typo.
255-305: LGTM!The
Statenamespace properly models form field state with discriminated unions for valid/invalid states and comprehensive readonly/readwrite state wrappers.
307-347: LGTM!The
Snapshotnamespace correctly models snapshot representations with proper value and error mappings for all field types.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
example/src/domain/Full.js (2)
81-85: Consider removing redundant Boolean cast.The
snapshot.valueis already typed correctly from theSnapshot.FromObjectFieldstype, soBoolean(snapshot.value)on line 84 appears redundant. The value should already be properly typed.Apply this diff if you want to remove the cast:
function handleSubmit(snapshot) { if (snapshot.invalid) return - setSubmitted(Boolean(snapshot.value)) + setSubmitted(snapshot.value) }
255-259: Consider importing from shared location.The
asConstandthrowErrorhelpers are duplicated here but also exist insrc/type-helpers.js(visible in the relevant code snippets). Consider importing these from the shared module to avoid duplication and maintain a single source of truth.Apply this diff to import instead of redefining:
+import { asConst, throwError } from './path/to/type-helpers' + // ... rest of file ... -/** @template const T @arg {T} x */ -function asConst(x) { return x } - -/** @arg {string} message @returns {never} */ -function throwError(message) { throw new Error(message) }Note: Adjust the import path based on the actual location of
src/type-helpers.jsrelative to this example file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
example/src/domain/Full.js(6 hunks)example/src/domain/machinery/Code.js(1 hunks)example/src/domain/machinery/Form.js(10 hunks)example/tsconfig.json(1 hunks)example/types/global.d.ts(0 hunks)src/validation.js(1 hunks)
💤 Files with no reviewable changes (1)
- example/types/global.d.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- example/tsconfig.json
- example/src/domain/machinery/Form.js
🧰 Additional context used
🧬 Code graph analysis (1)
example/src/domain/Full.js (3)
src/validation.js (8)
optionalT(25-27)required(40-42)required(40-42)requiredT(35-37)error(82-82)minLength(61-64)src/type-helpers.js (1)
asConst(5-7)src/snapshot.js (4)
field(49-49)field(75-75)field(100-100)children(76-86)
🔇 Additional comments (10)
example/src/domain/machinery/Code.js (1)
1-1: ****The original review comment is based on incorrect information. TypeScript recognizes
@argas a valid JSDoc tag—it is an alias of@paramand treated identically for JSDoc-based type information.The Code.js annotation is correct and follows the project's consistent convention, with 82+ instances of
@argthroughout the codebase. No changes are needed.Likely an incorrect or invalid review comment.
src/validation.js (3)
29-37: LGTM! Type-level helper with runtime consistency.The
requiredTfunction correctly returns therequiredvalidator while providing compile-time type information through the type parameter. The pattern is consistent withoptionalT.
3-13: LGTM! Type mapping with reasonable coverage.The
TypeLookuptypedef correctly maps string literals to their corresponding types with proper support for primitives and arrays. The recursive array pattern and fallback case are well-designed.
40-74: LGTM! Comprehensive type annotations.The type annotations added to all validators are accurate and provide proper type safety:
- Generic template on
requiredfor flexible input types- Specific type constraints on numeric and string validators
- Structural typing for
minLength/maxLengthsupporting any object with alengthpropertyexample/src/domain/Full.js (6)
14-47: LGTM! Consistent migration to typed validators.The field definitions correctly migrate from untyped to typed validators:
optionalT('string')provides compile-time type information while maintaining the same runtime behavior (null)requiredTvariants preserve the required validator behaviorasConstwrapper correctly preserves literal types for TypeScript inference- The dynamic schema in
specialeToevoeging(lines 40-44) is well-designed with runtime validation viathrowError
49-49: LGTM! Proper type derivation from fields.The
FormTypetypedef correctly derives the form type from thefieldsobject usingField.ObjectFromObjectInput, providing end-to-end type safety.
93-98: LGTM! Proper component props typing.The JSDoc type annotations for the
Formuliercomponent props are accurate and provide good IDE support.
152-159: LGTM! Correct discriminated union handling.The use of
asConstforinitialValuepreserves literal types, and the'roodInfo' in fields/'groenInfo' in fieldschecks correctly discriminate between the union types at runtime.
168-184: LGTM! Proper helper component typing.Both
BedanktandConditionalcomponents have accurate JSDoc type annotations. The use ofField.Basic<boolean>for the Conditional field prop is appropriate.
186-253: LGTM! Comprehensive typing for field state helpers.The type annotations for
useSendSignalWhenIsVisitedand thegetIsVisitedhelper functions are thorough and accurate. The@ts-expect-erroron line 219 is necessary because TypeScript cannot verify the dynamic field.type lookup is exhaustive, even though the runtime logic correctly handles all field types.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
example/tsconfig.json (1)
29-29: Consider reducingmaxNodeModuleJsDepth.Setting
maxNodeModuleJsDepthto10is unusually high (default is0). This instructs TypeScript to analyze JavaScript innode_modulesup to 10 levels deep, which may impact type-checking performance without providing significant benefit, since dependencies typically ship their own types.Consider reverting to a lower value (e.g.,
1or omitting it) unless there is a specific reason to deeply analyze dependency sources.example/src/domain/Full.js (1)
255-259: Consider importingasConstandthrowErrorfrom the library.These utility functions are redefined locally but appear to be available in the library's type-helpers module (as seen in
src/type-helpers.js). To reduce code duplication and follow DRY principles, consider importing them instead:+import { asConst } from '@kaliber/forms/type-helpers' + /** * When you study this example, don't forget to check the components in the machinery directory */Then remove the local definitions:
- -/** @template const T @arg {T} x */ -function asConst(x) { return x } - -/** @arg {string} message @returns {never} */ -function throwError(message) { throw new Error(message) }If
throwErroris not exported from the library, you could keep that one local while importingasConst.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
example/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
example/package.json(1 hunks)example/src/domain/Full.js(6 hunks)example/tsconfig.json(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
example/src/domain/Full.js (7)
src/fields.js (5)
fields(23-23)value(30-40)value(104-118)value(182-190)children(24-24)src/validation.js (8)
optionalT(25-27)required(40-42)required(40-42)requiredT(35-37)error(82-82)minLength(61-64)example/src/domain/machinery/validation.js (4)
date(5-5)date(5-5)ifParentHasValue(30-36)ifFormHasValue(15-20)src/type-helpers.js (1)
asConst(5-7)src/hooks.js (5)
form(30-34)useFormFieldValue(128-130)field(146-146)field(187-187)field(198-198)src/components.js (1)
FormFieldValid(35-38)src/snapshot.js (4)
field(49-49)field(75-75)field(100-100)children(76-86)
🔇 Additional comments (14)
example/tsconfig.json (3)
17-18: Verify strict type checking on existing JavaScript files.The combination of
checkJs: true,noImplicitAny: true, andstrictNullChecks: truewill enforce strict type checking on all JavaScript files in the example folder (src, config). Existing JS code without type annotations may fail these checks.Confirm that existing example code (if any) has been audited or updated to satisfy these stricter constraints, or consider relaxing these flags for
.jsfiles if necessary.
2-5: Good: typeAcquisition correctly excludes the local library.Excluding
@kaliber/formsfrom type acquisition is correct, since the library's types are now provided locally in this PR rather than from DefinitelyTyped.
6-7: TypeScript configuration aligns with PR's type system introduction.The configuration updates support the new TypeScript types:
- Paths and module resolution (
node16) align with the PR's typing strategy.allowImportingTsExtensionsandnoEmitallow type-checking.tsfiles without runtime concerns.allowJs,checkJs, and strict null/any checks enforce type safety across the example codebase.Also applies to: 9-10, 15-16, 22-24
example/package.json (1)
8-9: NODE_OPTIONS flag addition is appropriate for symlinked peer dependencies.The
--preserve-symlinksoption aligns with the postinstall peer dependency installation into atypescript/folder (as described in the PR summary). This ensures the build and serve processes correctly resolve symlinked dependencies.example/src/domain/Full.js (10)
2-2: LGTM! Type helper imports enhance type safety.The additions of
optionalTandrequiredTvalidation helpers along with the JSDoc type imports forFieldandSnapshotproperly support the TypeScript typing additions throughout this example.Also applies to: 7-7
14-14: LGTM! Field type annotations are correctly applied.The typed validation helpers (
optionalT,requiredT) are used appropriately for their respective field types, and the JSDoc parameter annotation on line 45 adds useful type safety to the custom validator.Also applies to: 17-18, 37-37, 45-45
19-27: LGTM! TheasConstwrappers properly preserve literal types.Wrapping the object field definitions with
asConstenables TypeScript to infer more precise literal types for heterogeneous structures, which is essential for type-safe form handling.Also applies to: 28-36
48-49: LGTM! Type derivation from fields definition is elegant.Using
Field.ObjectFromObjectInput<typeof fields>to deriveFormTypeprovides a single source of truth for the form structure, improving type safety and maintainability.
57-58: LGTM! Snapshot type derivation ensures type safety.The type annotations correctly derive snapshot types from
FormType, ensuring thatsubmittedand thehandleSubmitparameter are properly typed according to the form structure.Also applies to: 81-81
93-98: LGTM! Component prop types are comprehensive.The JSDoc type annotations for component props provide clear type contracts and enable type checking for
Formulier,Bedankt, andConditionalcomponents.Also applies to: 168-168, 180-180
152-158: LGTM! Type discrimination using theinoperator is type-safe.Using the
inoperator to check for the presence ofroodInfoorgroenInfois the idiomatic TypeScript pattern for discriminating union types at runtime. Combined with theasConst-wrappedinitialValueobjects, this provides both type safety and correct runtime behavior.
162-162: LGTM! Empty string for cursor is cleaner.Using an empty string for the cursor style when the form is valid allows the browser's default cursor to apply naturally, which is cleaner than explicitly setting
'default'.
186-189: LGTM! JSDoc annotations improve helper function type safety.The comprehensive JSDoc type annotations for
useSendSignalWhenIsVisited,getIsVisited, and related helper functions provide clear documentation and enable type checking for these utility functions.Also applies to: 210-213, 223-225, 229-229, 235-237, 241-241, 247-249
219-220: LGTM! The@ts-expect-errorsuppression is justified.TypeScript cannot always narrow discriminated union types when using the discriminator as a dynamic object key. The
@ts-expect-errordirective appropriately suppresses this limitation while the runtime behavior remains correct.
|
De suggesties zijn verwerkt en ook de example maakt nu gebruik van de typed versie en de nieuwe Laatste stap voor release: package maken (tarball) en in de node_modules map van een project zetten. Let op: grote kans dat je de |
|
Ik heb de tarball getest en dat werkt :-D Dus wat mij betreft kan die gereleased worden. Is wel een major version release. @AlbertSmit en @buildersteven4, waneer denken jullie even een blik te kunnen werpen? |
Improve typing support
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/validation.js (1)
39-51: Previously discussed — design confirmed by author.The overload signature returning
neverwas flagged in a prior review, and the author confirmed this is intentional for type-level inference purposes.
🧹 Nitpick comments (1)
src/validation.js (1)
30-37: Type-level helper returning null — document the intended usage.
withTreturnsnullat runtime but declaresConstructable<T>as its return type. This is a valid pattern for type-level helpers where the return value is only used for type inference. Consider adding a brief JSDoc comment explaining this is a type-only helper to prevent confusion for library consumers.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/types.tssrc/validation.jsvalidation.js
🔇 Additional comments (16)
validation.js (1)
1-7: LGTM!The expanded exports cleanly expose the new typed variants (
requiredT,optionalT,withT) alongside the existing validators. This is a straightforward, additive API surface change.src/validation.js (4)
18-28: TypeLookup only splits on first|— nested unions may not parse correctly.The template literal pattern
`${infer A} | ${infer B}`matches only the first|occurrence. For types like'string | number | boolean', this would inferA = 'string'andB = 'number | boolean', then recursively callTypeLookup<'number | boolean'>. This works due to recursion, but the pattern relies on left-to-right splitting.This is acceptable if intentional, but be aware that deeply nested unions will recurse multiple times. Consider documenting this behavior if consumers will use complex union strings.
53-61: LGTM!
requiredTcorrectly wraps the existingrequiredvalidator with proper type inference based on the provided type parameter.
63-98: LGTM!The JSDoc type annotations for all validators (
required,number,min,max,minLength,maxLength,
100-106: LGTM!The updated template using
{[...any]} Pfor rest parameters correctly captures variadic arguments as a tuple type, improving type inference for error parameters.src/types.ts (11)
5-6: LGTM!The
Expandutility type is a standard and effective pattern for improving type readability in IDE tooltips by flattening intersection types.
12-22: LGTM!
PartialWithStringKeycorrectly handles recursive partial mapping for objects and arrays while ensuring keys remain strings. The nested conditional logic properly distinguishes between array and object types.
26-30: LGTM!
MapTuplecorrectly implements recursive tuple mapping with proper inference and rest element handling.
32-47: LGTM!The validation type hierarchy is well-structured.
ValidateToValuecorrectly handles various validation function arrangements (single, tuple, null) with proper type extraction and intersection for multiple validators.
55-67: LGTM!The
FieldInputnamespace cleanly defines the input types for objects and arrays, including support for heterogeneous arrays via constructor functions.
69-111: LGTM!The
FieldSchemanamespace provides a comprehensive type hierarchy for field schemas with proper value extraction throughToValueand its helpers. The recursive type mappings correctly handle nested object and array structures.
113-179: LGTM!The
NormalizedFieldnamespace correctly maps fromFieldSchemato normalized field representations. TheFromFieldSchemaconditional type comprehensively handles all schema variants including the edge case of basic fields with explicit validate property.
193-194: Typo has been fixed.The type is now correctly named
ObjectFromObjectInput(previously flagged asObjecFromObjectInputin a past review).
181-260: LGTM!The
Fieldnamespace is well-structured with proper type mappings.BaseFieldPropertiescorrectly defines the common interface for all field types, and the specializedBasic,Object, andArraytypes extend it appropriately with their specific properties.
262-312: LGTM!The
Statenamespace provides a clean discriminated union for valid/invalid states via theCommon & (Valid | Invalid)pattern. TheReadonly,ReadonlyWithHistory, andReadWriteinterfaces properly model the state access patterns with appropriate subscription types.
314-354: LGTM!The
Snapshotnamespace correctly models the snapshot structure with proper value and error extraction. TheFromFieldtype mapping andMapToValues/MapToErrorshelpers provide clean recursive type transformations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/snapshot.type.test.ts (1)
131-133: Incomplete test block—missing type assertions.This block extracts the snapshot result but doesn't verify its type with
expectNotAny,expectNotNever, orexpectAssignable. Consider adding assertions for consistency with other test blocks.🔎 Suggested fix
{ const result = snapshot.get(form) + expectNotAny(result) + expectNotNever(result) + // Add expectAssignable with the expected full form snapshot shape }src/hooks.js (1)
12-23: Consider stronger typing foronSubmitcallback.The
onSubmitparameter is typed as(snapshot: any) => void. Since the form structure is known via genericA, the snapshot type could be derived for better type safety.🔎 Potential improvement
* @arg {{ * fields: A, * initialValues?: Expand<InitialValue<A>>, * validate?: C, - * onSubmit: (snapshot: any) => void, + * onSubmit: (snapshot: Expand<Snapshot.FromField<Field.ObjectFromObjectInput<A>>>) => void, * formId?: string, * }} propsThis would propagate type information to consumers of the form snapshot.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components.type.test.tssrc/hooks.jssrc/hooks.type.test.tssrc/snapshot.type.test.ts
🧰 Additional context used
🧬 Code graph analysis (4)
src/snapshot.type.test.ts (6)
src/hooks.js (2)
useForm(24-55)form(30-34)src/validation.js (2)
optionalT(49-51)requiredT(59-61)src/schema.js (2)
object(19-21)array(39-41)src/type-helpers.js (1)
asConst(5-7)src/type.test.helpers.ts (3)
expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)src/types.ts (2)
Falsy(24-24)ValidationError(46-46)
src/hooks.js (3)
src/type-helpers.js (1)
asAny(10-12)src/components.js (1)
useFormFieldSnapshot(36-36)src/snapshot.js (3)
field(49-49)field(75-75)field(100-100)
src/hooks.type.test.ts (5)
src/hooks.js (9)
useForm(24-55)useFormFieldValue(128-130)useFormFieldsValues(136-138)useFormField(144-150)useNumberFormField(155-167)useBooleanFormField(170-180)useObjectFormField(197-202)useArrayFormField(186-191)useFormFieldSnapshot(112-121)src/validation.js (1)
optionalT(49-51)src/schema.js (2)
object(19-21)array(39-41)src/type.test.helpers.ts (1)
expectNotAny(12-12)src/types.ts (8)
Field(181-181)Basic(163-166)Basic(222-232)Basic(283-285)Basic(326-327)State(262-262)Event(234-238)Snapshot(314-314)
src/components.type.test.ts (5)
src/hooks.js (2)
useForm(24-55)form(30-34)src/validation.js (1)
optionalT(49-51)src/schema.js (2)
object(19-21)array(39-41)src/components.js (4)
FormFieldValue(13-16)value(14-14)FormFieldsValues(24-27)FormFieldValid(35-38)src/type.test.helpers.ts (4)
expectNotAny(12-12)expectNotNever(2-2)expectAssignable(15-15)Prepared(28-29)
🪛 Biome (2.1.2)
src/snapshot.type.test.ts
[error] 9-9: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/hooks.js
[error] 10-10: This hook is being called indirectly and conditionally, but all hooks must be called in the exact same order in every component render.
This is the call path until the hook.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 24-24: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/hooks.type.test.ts
[error] 17-17: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 57-57: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 60-60: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 63-63: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 77-77: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 91-91: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 105-105: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 119-119: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
[error] 144-144: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
src/components.type.test.ts
[error] 7-7: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (8)
src/snapshot.type.test.ts (1)
1-32: Type test setup looks good.The form definition with diverse field types (primitives, nested object, array, and dynamic array with discriminant) provides thorough coverage of the snapshot typing system. The Biome warning about hooks being called conditionally is a false positive—this is a type test file, not a React component.
src/components.type.test.ts (1)
1-74: Well-structured component type tests.The tests thoroughly validate the typing for
FormFieldValue,FormFieldsValues, andFormFieldValidrender prop components. Type assertions correctly match field definitions. The Biome warning is a false positive—this is a type test file, not a React component.src/hooks.type.test.ts (2)
134-138: Verifyremovehelper field types includeundefined.The original fields are defined with
optionalT, meaning types should beField.Basic<string | undefined>,Field.Basic<number | undefined>, etc. The assertion here uses non-optional types which may cause type mismatches.🔎 Potential fix
remove(entry: Field.Object<{ - a: Field.Basic<string> - b: Field.Basic<number> - c: Field.Basic<boolean> + a: Field.Basic<string | undefined> + b: Field.Basic<number | undefined> + c: Field.Basic<boolean | undefined> }>): void
1-164: Comprehensive hook type coverage.The test file provides good coverage for all form-related hooks with proper type assertions. The Biome warnings about conditional hook calls are false positives since this is a type test file, not a React component.
src/hooks.js (4)
24-55:useFormimplementation with enhanced typing looks correct.The previous typo (
ObjecFromObjectInput) has been fixed. The lazy initialization pattern withReact.useRefand equality check for re-initialization is appropriate. The Biome warning about conditional hook calls is a false positive—useFormId()in the default parameter is a valid pattern and evaluates unconditionally when noformIdis provided.
62-74: Lazy state initialization pattern is correct.Using
React.useState(state.get)(function reference, not invocation) correctly leverages React's lazy initialization to defer computing the initial value until needed.
76-105:useFieldStatesimplementation is well-typed.The return type cast to
State.StateTupleToValueTuple<T>correctly maps the state tuple to values. The comment at lines 204-212 explains the rationale for passing arrays directly as dependencies.
107-202: Hook implementations with JSDoc templates look solid.All field hooks (
useFormFieldSnapshot,useFormFieldValue,useFormFieldsValues,useFormField,useNumberFormField,useBooleanFormField,useArrayFormField,useObjectFormField) are properly annotated with templates and return types. The@satisfiesannotation on line 114 ensures the memoized state object conforms toState.Readonly.
| const formFieldValue = useFormFieldValue(form.fields.a) | ||
| expectAssignable<string, Prepared<typeof formFieldValue>> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/hooks.type.test.ts | head -100Repository: kaliberjs/forms
Length of output: 3306
🏁 Script executed:
rg -A 15 'function useFormFieldValue' src/Repository: kaliberjs/forms
Length of output: 606
🏁 Script executed:
rg -A 10 'useFormFieldValue' src/hooks.ts src/hooks.d.ts 2>/dev/null || rg -A 10 'export.*useFormFieldValue' src/Repository: kaliberjs/forms
Length of output: 486
🏁 Script executed:
fd -e "d.ts" -e "ts" src/hooksRepository: kaliberjs/forms
Length of output: 41
🏁 Script executed:
cat -n src/types.ts | head -150Repository: kaliberjs/forms
Length of output: 6094
🏁 Script executed:
cat -n src/types.ts | sed -n '150,300p'Repository: kaliberjs/forms
Length of output: 5291
🏁 Script executed:
rg -A 10 'function useFormFieldState' src/Repository: kaliberjs/forms
Length of output: 450
🏁 Script executed:
cat -n src/validation.ts | grep -A 5 "optionalT"Repository: kaliberjs/forms
Length of output: 108
🏁 Script executed:
fd -e "ts" -e "js" | xargs grep -l "optionalT"Repository: kaliberjs/forms
Length of output: 194
🏁 Script executed:
cat -n src/validation.js | grep -B 5 -A 10 "optionalT"Repository: kaliberjs/forms
Length of output: 512
🏁 Script executed:
rg -B 5 -A 15 "type Prepared" src/type.test.helpers.tsRepository: kaliberjs/forms
Length of output: 742
🏁 Script executed:
cat -n src/hooks.js | sed -n '1,100p' | grep -B 10 -A 5 "useFormFieldValue"Repository: kaliberjs/forms
Length of output: 41
🏁 Script executed:
rg -B 3 -A 8 "useFormFieldValue" src/hooks.jsRepository: kaliberjs/forms
Length of output: 338
Type assertion is incorrect—expected type should include undefined.
Field a is defined as optionalT('string'), which yields Field.Basic<string | undefined>. The assertion at line 58 expects only string, but useFormFieldValue returns the field's value state, which has type string | undefined. This is inconsistent with the similar test at line 60-61, which correctly includes undefined for optional fields. Change expectAssignable<string, ...> to expectAssignable<string | undefined, ...>.
🧰 Tools
🪛 Biome (2.1.2)
[error] 57-57: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🤖 Prompt for AI Agents
In src/hooks.type.test.ts around lines 57 to 58, the type assertion expects a
plain string but field `a` is optional (optionalT('string')) so the returned
value can be string | undefined; update the assertion to expect `string |
undefined` instead of `string` so the test matches the optional field's type.
Summary by CodeRabbit
New Features
Refactor
Chores
Removed
✏️ Tip: You can customize this high-level summary in your review settings.