Skip to content

Add better typing capabilities#823

Open
synapseradio wants to merge 2 commits into
masterfrom
feature/better-types
Open

Add better typing capabilities#823
synapseradio wants to merge 2 commits into
masterfrom
feature/better-types

Conversation

@synapseradio

Copy link
Copy Markdown
Collaborator

If merged, will lead to a minor release as it should be backwards compatible

feat(formal):

  • replace eslint with rome
  • add better typings that allow for explicit markup when provided

Rationale

Why is this PR necessary?

This PR adds type annotations so that formal can infer better types for createRule and all other related functions.

It does not introduce mandatory types in new places ( unless you are manually instantiating Success or Fail classes, which you probably aren't )

these annotations can lead to typescript helping you out a lot more as you create custom rules, and in some cases, not have to run to see if things failed when you're using pipeValidators or validateObject.

They still fall back to very forgiving types that are effectively the same as before ( when most types were casted to any without the ability to specify ).

consider the examples below, pulled from the tests:

const hasExactlyTwoNameObjects = createRule({
	condition: (arr: string[]) => arr.length === 2,
	message: 'Exactly two names must exist',
})

because the condition is marked with a param type, this rule will now correctly infer if you're trying to use it on some known other type by mistake, without having to wait until a failure at runtime to be certain.

it still works without complaint the way it did before if the annotation is gone. ( the same augmentation has happened with the optional transform fn )

const overwriteText = withMessage<number>('Whoa')

Same effect with withMessage.

Since rules cascade over a single value with a single type, you can now put the type into the Validator class and the pipeValidators util.

Validator.of<string, OptionalOnSuccessReturnType, OptionalOnFailReturnType>(isNonEmptyString).validate('wow').then({
  onSuccess: (v: string) => OptionalOnSuccessReturnType, // string value is inferred, you don't have to type it out
  onFail: (errors: string[]) => OptionalOnFailReturnType // onFail will Always have the errors as the argument rather than the value, but this was the same as before
})

with these optional type annotations ( they all default to any ) you can get better guarantees of what will happen next with the return from this validator.

Rome has been added as a formatter / linter since it is much lighter and much faster than the eslint rules were. Any CI dealing with this package should be faster on the order of 40-60x. It makes for a messy diff where strings are getting their quotes changed, but it makes linting a whole lot easier and closer to the more standard style of prettier. it's abstracted behind the lint and style commands. Because the eslint dependencies are gone, the installation should take a lot less time and the lockfile is shorter.

Rome has a pretty nice VS code extension as well if you're developing and want it to format on save.

I have used pnpm as a package manager ( which I highly suggest switching to ) but have also updated yarn.lock for compatibility with CI.

@synapseradio synapseradio requested review from a team and lkapt March 13, 2023 20:18
@synapseradio synapseradio force-pushed the feature/better-types branch from f8619a5 to be7ae63 Compare March 13, 2023 20:32

@nicolam1 nicolam1 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review: Add better typing capabilities

Thanks for this PR @synapseradio — the goal of improving type inference across the formal API is a great one. The general approach of adding generic type parameters to , , , , and the various type aliases in is sound, and the backward-compatibility story (defaulting to ) is well-considered. That said, I have several concerns that I think should be addressed before merge.


🔴 Critical: Type safety holes in the implementation

1. is still typed as despite the generic

In , the class declaration is , but the instance property is still:

This defeats the purpose of the generic. The field should be typed as (or if is a valid initial state). As-is, the generic on is purely decorative — callers get no type-narrowing from it, and still returns an object whose is .

The same issue applies to where — this means must accommodate , or the assignment is unsound. Consider with definite assignment assertion, or make the constructor parameter and remove the field initializer.

2. return type is wrong

The interface declares:

But the implementation just calls , which returns , not . The return type should be (or possibly if you want to be conservative). As declared, TypeScript will believe on a returns when it actually returns .

3. uses unnecessarily and inconsistently

The signature is:

Using here means the callback receives a narrowed type, but is (due to the initializer), so there's a disconnect. If you intend to never be null at chain time, the field itself should reflect that (e.g., via definite assignment). As-is, the annotation is lying — it claims the callback won't receive null, but the runtime value could be null.

4. has an implicit return path

The method on :

The catch block has no statement, so the function implicitly returns , but the declared return type is . This is a type safety bug. Either the catch should re-throw, return a , or the return type should be .

5. in fold call

This suppresses a real type error. The issue is that is a union of and , and has different return types on each branch. Rather than suppressing the error, consider making return , or use a discriminated union approach where narrows based on . Suppressing the error hides the fact that the return type of ( per the interface) doesn't match reality.


🟡 Moderate concerns

6. uses cast

evaluates to , so this cast is — it's a type lie that bypasses the type system entirely. This appears to be a workaround for the fact that infers its type argument from the input, but you want the transform to change the type. A cleaner approach would be to have accept a type parameter explicitly: .

7. and have unused generic parameters

has and that are never referenced in the type body — they default to and aren't used anywhere in the function signature. This adds complexity without benefit. Consider removing them unless there's a planned use case.

8. still uses for the inner function's parameter

Even though is now generic in , the returned function's parameter is typed as rather than . This breaks the type chain — callers who pass a typed value will lose type information here.

9. Built-in rules don't carry type information

The built-in rules like , , etc. still use without explicit type parameters. For example, 's condition takes with no type annotation. Since now infers the from , these rules will infer as their value type rather than , , etc. This means using them in a won't give you type errors if you pass a number. Consider adding explicit type annotations to the built-in rules' condition functions.


🟢 Stylistic / minor

10. Linter/tooling change mixed with type changes

This PR mixes two concerns: (a) replacing ESLint with Rome and (b) adding generic type parameters. The Rome change alone touches every source file with quote-style and formatting changes, making it very hard to isolate which changes are type-related vs. formatting. These would be easier to review as separate PRs.

11. adds 5600 lines

Consider whether the lockfile should be in the repo if yarn.lock is also maintained for CI compatibility. Having both can lead to drift.

12. return type in interface

The interface declares returns , but the implementation returns whatever returns (which is ). The interface return type doesn't match the implementation reality.


Summary

The direction of adding generics to the core types is right, but the implementation has several type-safety holes that undermine the stated goal. The most impactful issues are: on , incorrect return types, the implicit return in , and the cast in . I'd recommend fixing items 1–5 before merging, and strongly consider addressing 6 and 8 as well.

@nicolam1

Copy link
Copy Markdown

Detailed Review: Add better typing capabilities (PR #823)

Thanks for this PR @synapseradio — improving type inference across the formal API is a worthwhile goal. The approach of adding generic type parameters to Validator, Success, Fail, Rule, and the type aliases in types/index.ts is sound, and defaulting to any for backward compatibility is well-considered. However, I have several concerns that should be addressed.


🔴 Critical: Type safety holes

1. Fail.value is still typed as any despite the ValueType generic

In src/fail.ts, the class declaration is Fail<ValueType, OnFailReturns>, but:

value: any = null

This defeats the purpose of ValueType. The field should be typed as ValueType (or ValueType | null). As-is, the generic on Fail is purely decorative — Fail.of<string>('hello') still returns an object whose .value is any.

The same issue applies to Success<ValueType> where value: ValueType = null — this means ValueType must accommodate null, or the assignment is unsound. Consider value!: ValueType with a definite assignment assertion, or make the constructor parameter public value: ValueType.

2. Success.fold return type is wrong

The Success interface declares:

fold(opts: ValidationActions<ValueType, OnSuccessReturns, void>): ValueType

But the implementation calls onSuccess(this.value) which returns OnSuccessReturns, not ValueType. The return type should be OnSuccessReturns. TypeScript will believe fold on Success<string, number> returns string when it actually returns number.

3. Success.chain uses NonNullable<ValueType> inconsistently

chain(fn: (v: NonNullable<ValueType>) => ValidationM<ValueType>): ValidationM<ValueType>

Using NonNullable<ValueType> means the callback receives a narrowed type, but this.value is ValueType | null (due to = null), so there's a disconnect. If value should never be null at chain time, the field should use definite assignment. As-is, NonNullable is lying about what the callback receives.

4. Fail.chain has an implicit undefined return path

chain(fn: ...): Fail<ValueType, OnFailReturns> {
    try {
        const result = validationM(this.value, this.errors)
        checkIsValidationM(result)
        return new Fail(result.value, [...])
    } catch (error) {
        console.error(error.message)
        console.error(error.stack)
        // no return! implicitly returns undefined
    }
}

The declared return type is Fail<ValueType, OnFailReturns> but the catch block has no return. This is a type safety bug. Either re-throw, return a Fail, or change the return type to Fail<...> | undefined.

5. @ts-expect-error in Validator.then fold call

// @ts-expect-error Success and Fail have different returns but it's ok here
return this.result.fold(opts)

This suppresses a real type error. ValidationM is a union of Success and Fail, and fold has different return types on each branch. Rather than suppressing the error, consider making then() return OnSuccessReturns | OnFailReturns, or use a discriminated union where then narrows based on result.isSuccess.


🟡 Moderate concerns

6. Rule.check uses as Exclude<never, TransformReturns> cast

return Success.of(this.opts.transform(value) as Exclude<never, TransformReturns>)

Exclude<never, TransformReturns> evaluates to never, so this is effectively value as never — a type lie that bypasses the type system. A cleaner approach: Success.of<TransformReturns>(this.opts.transform(value)).

7. ValidationCheck has unused generic parameters

ValidationCheck<ValueType, FormValues, OnSuccessReturns, OnFailReturns>OnSuccessReturns and OnFailReturns default to any and are never referenced in the type body. Consider removing them unless there's a planned use case.

8. pipeValidators inner function still uses any

(value: any) => {

Even though pipeValidators is now generic in ValueType, the returned function's parameter is any rather than ValueType. This breaks the type chain.

9. Built-in rules don't carry type information

Built-in rules like isNonEmptyString use createRule without explicit type parameters. The condition (maybeStr) => typeof maybeStr === 'string' ... has no type annotation, so ValueType infers as any. Consider adding explicit type annotations: condition: (maybeStr: string) => ....


🟢 Stylistic / minor

10. Linter change mixed with type changes — This PR mixes ESLint→Rome migration (touching every file with formatting) with type system changes. Separate PRs would make review much easier.

11. pnpm-lock.yaml adds 5600 lines — Having both pnpm-lock.yaml and yarn.lock can lead to drift. Consider which is canonical.

12. Validator.then return type mismatch — Interface says then() returns ValueType, implementation returns OnSuccessReturns | OnFailReturns. These should align.


Summary

The direction is right, but several type-safety holes undermine the stated goal. Items 1-5 (especially value: any on Fail, incorrect fold return types, implicit undefined return in Fail.chain, and the as never cast) should be fixed before merging. Items 6 and 8 also deserve attention.

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.

3 participants