Use `interface` instead of `type` for all Base types #259

Saeris · 2023-11-19T16:44:31Z

See #257 for background.

While this PR touches many files, the bulk of the incoming changes here simply convert type intersections to interface declarations. The intent here is to enable third-party developers to easily pattern match against BaseSchema/BaseSchemaAsync when building type guards, or further refine types to specific schema subtypes such as NumberSchema.

In resolving type errors that came up as a result of these changes within Valibot's codebase, I also made some improvements to the implementations of some utility functions such as getDefault and getFallback. These functions and their related types were simplified while improving their own internal type safety.

Please read the comments below for more context.

netlify · 2023-11-19T16:44:36Z

✅ Deploy Preview for valibot ready!

Name	Link
🔨 Latest commit	`3b2d314`
🔍 Latest deploy log	https://app.netlify.com/sites/valibot/deploys/656f63ba27315e00081aa1b6
😎 Deploy Preview	https://deploy-preview-259--valibot.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

fabian-hiller · 2023-11-19T16:53:40Z

Please have a look at #257 (comment)

Using some basic type predicate functions for base schemas, object schemas, and tuple schemas, I refactored the fallback and default methods which simplifies both their types and the logic for each method. Note that because each of these methods relied on a mutable return value typed as `any`, Typescript did not catch any logic errors when calculating default or fallback values. With the refactors, in some cases an `as` type assertion is necessary. For some of the method types, I reorganized them such that they no longer introduce a circular dependency (even if only at the type level).

Saeris

Added some context for the refactors I did tonight. Let me know what you think.

library/src/methods/fallback/fallback.ts

Saeris · 2023-11-21T09:20:26Z

library/src/methods/fallback/types.ts

Both of these interfaces could be simplified because the Output<> utility type would just extract the TOutput type that the BaseSchema was instantiated with.

library/src/methods/getDefaults/getDefaults.ts

Saeris · 2023-11-21T09:40:17Z

library/src/methods/getDefaults/getDefaults.ts

+  // Otherwise, check if schema is of kind object or tuple
+  // If it is an object schema, set object with default value of each entry
+  if (isObjectSchema(schema)) {
+    return Object.entries(schema.entries).reduce(


This could stay a for loop instead of a reduce. I did it this way out of habit. One thing to note is that Object.entries() here is preferable because the in operator can have unexpected results due to it including properties from an object's prototype chain.

See difference between for...of and for...in

If using Object.fromEntries in the async version, why not use it here as well? Is it because of the performance cost of creating an extra array?

I suppose it can be done with Object.fromEntries here too, since in either case a type assertion is required. I implemented one after the other and solved for type errors along the way, so these were just the solutions I arrived at then. Will make this change when I resolve merge conflicts.

library/src/methods/getDefaults/getDefaultsAsync.ts

Saeris · 2023-11-21T09:47:09Z

library/src/schemas/object/isObjectSchema.ts

If it's really necessary I can add a test for this type predicate function. It already has coverage thanks to the tests for the refactored methods.

Saeris · 2023-11-21T09:51:04Z

library/src/utils/isNonNullable.ts

This is a utility I use in my own libraries. I can copy over the unit test for this too after an initial review.

Saeris · 2023-11-21T09:52:45Z

library/src/utils/isSchema.ts

I can add an explicit unit test for this type predicate but I believe it is already well covered by the schema method tests.

It may be worth adding two more functions for Validations and Transforms as a utility for third-party library authors.

fabian-hiller · 2023-11-22T15:24:40Z

I will try to take a closer look at this PR this weekend and give you feedback afterwards. Please don't put too much work into this PR until it's clear how we're going to proceed. I deliberately chose types instead of interfaces initially because I had negative experiences with them in Modular Forms.

In each case here, `TRest` can possibly be `undefined`, which means the resulting type intersection `(BaseSchema | BaseSchemaAsync) & TRest` where `TRest` is `undefined` causes the whole intersection to be `undefined`. To solve for this, we can instead compare `TRest` to `NonNullable<TRest>` to narrow it down to `BaseSchema | BaseSchemaAsync`.

This is a bugfix for an issue on main

fabian-hiller · 2023-11-29T04:40:03Z

I had a look at this PR and want to thank you for the work you put into it. In general I prefer to use type, but in the case of Valibot it may make sense to switch to interface for some definitions. Before I make a decision, I would like to hear your opinion on #258 and what advantages you see in interface and if you have encountered any disadvantages during implementation.

fabian-hiller · 2023-12-04T04:30:21Z

Can you respond to my previous comment and explain all the advantages you see in interface and also address possible disadvantages?

Saeris · 2023-12-04T07:57:16Z

Can you respond to my previous comment and explain all the advantages you see in interface and also address possible disadvantages?

Hey, I intend to get back to you this week. I was busy this past week studying for a Japanese language exam. Sorry for the delay!

fabian-hiller · 2023-12-04T14:32:38Z

No problem, there's no rush.

Saeris · 2023-12-05T04:28:32Z

Okay, here are a few reasons why I think interface is preferable to type in a library like Valibot:

To speak directly to the original issue, this change is being proposed as a way to help 3rd-party library authors to build their own libraries on top of Valibot. This is because interface supports Declaration Merging, an important feature that provides that developer a means to add new functionality to Valibot with their library without necessitating the end user to adopt usage of a type alias when consuming the library. Which could be further complicated if multiple libraries that extend Valibot are used, where this quickly becomes a mess for the end user to unravel. I think Declaration Merging has a good use-case too in that an end-user can add support for their project that might not make sense to contribute upstream to Valibot itself.
Another consideration from Types vs Interfaces in the official docs is that:

Interface names will always appear in their original form in error messages

I think it's important for the developer experience that when an error crops up, you can trace it back to its source quickly. Because of this behavior with interface names always being present is that at a glance, you know where to look next when a type error occurs. Contrast that with type (and in my experience this was historically an issue with type intersections especially) in that you might not know where the type conflict originates. When there are multiple type intersections in the chain, especially with arbitrary anonymous types, you'll have to dig for the property you have the conflict with.

There is a perf cost associated with the two approaches where the type checker is concerned (meaning, it could affect editor tooling). I ran tsc --noEmit --extendedDiagnostics on my library discordkit, which I am refactoring away from zod, and compared that build to a local build of this PR (w/Interfaces) and v0.21.0 (w/Types) as a benchmark. One significant difference I noticed between these 3 builds is that w/Interfaces has less that 1/5th the Instantiations than the w/Types build does, and is about 30% less than w/Zod. The rest of the differences aren't as pronounced, though the Check Time for w/Interfaces was about 1s faster than the other two.

	w/Zod	w/Interfaces	w/Types
Files:	2055	2172	2172
Lines of Definitions:	314503	351242	351142
Lines of TypeScript:	21216	22891	22891
Lines of JavaScript:	302	302	302
Lines of JSON:	0	0	0
Lines of Other:	0	0	0
Identifiers:	476096	434391	433892
Symbols:	643182	525129	522803
Types:	158165	71237	69277
Instantiations:	1338374	943901	5405629
Memory used:	800925K	658503K	676255K
Assignability cache size:	67049	19106	20834
Identity cache size:	1486	746	632
Subtype cache size:	986	1744	1869
Strict subtype cache size:	225	227	599
I/O Read time:	0.18s	0.20s	0.21s
Parse time:	1.01s	1.03s	0.99s
ResolveTypeReference time:	0.02s	0.02s	0.02s
ResolveLibrary time:	0.02s	0.01s	0.02s
Program time:	2.02s	2.88s	2.89s
Bind time:	0.53s	0.54s	0.57s
Check time:	3.28s	2.22s	3.52s
printTime time:	0.00s	0.00s	0.00s
Emit time:	0.00s	0.00s	0.00s
Total time:	5.83s	5.64s	6.98s

But i don't have a good understanding of what impact Instantiations has on the performance of the type checker. Personally I have noticed that the w/Types build has significantly slower IDE tooltips than w/Zod, which isn't present in w/Interfaces, but I'm unsure if this is related or not.

One other benchmark I ran was to compare the output of tsc --noEmit --generateTrace ./trace on w/Types and w/Interfaces so that I could go into Perfetto and drill into the stack traces. One of note was comparing the checkSourceFile trace on discordkit/packages/client/src/channel/__tests__/getchannelinvites.spec.ts where the execution duration on w/Types was 205ms vs 24ms on the w/Interfaces build.

Now I don't know much about perf testing the Typescript compiler much more than what I could read in a day, but the comparison that comes to mind for me is that if I were playing say, Dota 2, I would be at a significant disadvantage with a ping of 200ms vs my opponents if theirs was only 25ms. If this difference in checkSourceFile (which was consistently longer for all schema related files in w/Types) has any impact on the responsiveness I was experiencing with my IDE, then I think it's reasonable to assume that switching to using interface here (or some other related change I made) is having a meaningful impact on the developer experience of using Valibot.

Generally, the advice I've been seeing lately is, interface is still preferable to type for describing the shape of an object in TypeScript, especially if you're extending objects and/or you are authoring a library. In other contexts, it's more a matter of personal preference. If it's your own application code, fine, but if its library code, then it could create perf issues for consumers.

I couldn't find much supporting evidence for the performance impact of extends vs intersections (specifically coupled with generics), but my hunch is that TypeScript doesn't always cache type intersections (historically this was an issue) when generics are involved, so the pattern of BaseSchema & { ... } might cause a lot of extra instantiations. More digging is required on the huge difference in that metric. One example that did come up was a perf optimization where you need to extract out a complex conditional to its own named type so it can be cached. I had seen something similar in another article where an intersection was involved too, but I seem to have lost the link. I'll add it if I can find it. tl;dr was, if your intersection includes conditional types, extract the conditional to a named type with a generic, and intersect your base type with that type instead of doing it inline.

This article by the tRPC team was also an interesting read, and I recommend checking it out as got really buried in search results.

Another insightful article pointed out that Unions | and Intersections & do not behave like logical OR || or AND &&, which can result in some unintended gotchas. Objects defined via type also implicitly have an index signature, where interfaces do not, which can also lead to unexpected results.

fabian-hiller · 2023-12-05T16:59:44Z

Thank you for your detailed response. I will examine the code and make a decision by the end of the month.

fabian-hiller · 2024-01-05T02:23:22Z

Short update: I wasn't able to review this PR last month. It's planned to do so in the coming weeks.

ariskemper · 2024-01-05T12:42:22Z

Interfaces vs Types in TypeScript

Declaration Merging:
Interfaces: Can be merged. This is useful when you're extending existing interfaces or working with third-party TypeScript definitions.
Types: Cannot be merged. Each type alias is unique.

Extending:
Interfaces: Easily extended using the extends keyword, promoting reusability.
Types: Can be extended using intersections, but it's not as intuitive as with interfaces.
Implementation Specifics:

Interfaces: More suited for defining public API's of classes.
Types: Better for complex type transformations or when using union types.

Readability and Clarity:

Interfaces: May be more readable and clear for object-oriented patterns.
Types: Sometimes more concise, especially with complex type relationships.

Performance Considerations
In terms of performance, there is no significant difference between interfaces and types in TypeScript. TypeScript compiles down to JavaScript, and both interfaces and types are used for type-checking during development and do not exist in the compiled JavaScript code. Therefore, they don't have a runtime performance impact.

Recommendations
Use interfaces when you need to define the shape of objects and you anticipate extending these shapes or integrating with third-party type definitions.
Use types for more complex type transformations or when working with unions and intersections.
Consistency is key. Stick to one approach within a project as much as possible to maintain readability and ease of maintenance.

Saeris · 2024-01-05T17:16:55Z

Performance Considerations In terms of performance, there is no significant difference between interfaces and types in TypeScript. TypeScript compiles down to JavaScript, and both interfaces and types are used for type-checking during development and do not exist in the compiled JavaScript code. Therefore, they don't have a runtime performance impact.

@ariskemper This is inaccurate, because we are talking about Authoring Time and CI Time performance (IDE tooltips, typechecking), in which there are differences between the two. This PR is about improving the developer experience, which includes perf improvements that affect how responsive their coding environment is.

Saeris · 2024-01-05T17:18:32Z

I'll give this PR an update pass this weekend to merge in the recent changes on main.

…m/Saeris/valibot into feat/use-interface-for-base-types

fabian-hiller · 2024-03-01T17:23:22Z

library/src/methods/getDefault/types.ts

+/**
+ * Schema with maybe default type.
+ */
+export interface SchemaWithMaybeDefault<TInput = any, TOutput = TInput>


Please move this into getDefaults.ts

I moved these types because they created a circular dependency reference. IMO they are better kept in this file.

Why does it create a circular dependency reference? I prefer to move it to getDefaults.ts to be consistent with the rest of the source code. Types that are related to a particular function are placed above the function in the same file.

because for example:

export type DefaultValue< TSchema extends SchemaWithMaybeDefault | SchemaWithMaybeDefaultAsync, >

in types.ts would import from getDefaults.ts, which itself imports DefaultValue from types,ts

I will have a look at it. I don't think this is a problem but I could be wrong.

fabian-hiller · 2024-03-01T17:23:35Z

library/src/methods/getDefault/types.ts

+/**
+ * Schema with maybe default async type.
+ */
+export interface SchemaWithMaybeDefaultAsync<TInput = any, TOutput = TInput>


Please move this into getDefaultsAsync.ts

library/src/methods/getDefaults/getDefaults.ts

fabian-hiller · 2024-03-01T17:31:10Z

library/src/methods/getFallback/types.ts

+/**
+ * Schema with maybe fallback type.
+ */
+export interface SchemaWithMaybeFallback<TInput = any, TOutput = TInput>


Please move that into getFallback.ts.

fabian-hiller · 2024-03-01T17:31:21Z

library/src/methods/getFallback/types.ts

+/**
+ * Schema with maybe fallback async type.
+ */
+export interface SchemaWithMaybeFallbackAsync<TInput = any, TOutput = TInput>


Please move that into getFallbackAsync.ts.

library/src/utils/isSchema.ts

fabian-hiller · 2024-03-04T18:18:19Z

I will review this PR as well as #451 today and tomorrow and decide again if I want to change to interface.

Saeris · 2024-03-04T21:31:14Z

Just merged in the latest from the main branch, will address your feedback and requested changes as soon as I have a chance.

This latest commit adds the hasType utility we discussed in #250 (I may need to resolve some linting errors)

fabian-hiller

There are a few minor details we need to work out, but otherwise this PR seems ready to merge.

library/src/methods/fallback/fallback.ts

library/src/methods/fallback/fallbackAsync.ts

library/src/methods/getDefault/types.ts

fabian-hiller · 2024-03-06T00:33:50Z

I will do a final review later and probably merge this PR. 🙌

fabian-hiller · 2024-03-07T03:31:45Z

v0.30.0 is available

aboqasem · 2024-03-16T07:56:22Z

library/src/methods/getDefaults/getDefaults.ts

-  let defaults: any;
-
-  // If schema contains a default function, set its default value
+>(schema: TSchema): DefaultValues<TSchema> | undefined {


Hello, as per the documentation for getDefaults:

The difference to getDefault is that for objects and tuples without an explicit default value, this function recursively returns the default values of the subschemas instead of undefined.

Did that change?

No, it should work just the same. All unit tests passed, so I do not expect any bugs, but maybe we are missing something. Please create an issue if you encounter any problems.

Ok thanks. It just broke the type when I upgraded so I was wondering.

Feel free to share your code if you thing something is wrong on our side.

Use interface instead of type for all Base types

fd86987

fabian-hiller self-assigned this Nov 19, 2023

fabian-hiller added the enhancement New feature or request label Nov 19, 2023

Saeris mentioned this pull request Nov 19, 2023

Consider migrating from Type to Interface #257

Closed

Saeris commented Nov 21, 2023

View reviewed changes

Merge branch 'main' into feat/use-interface-for-base-types

cc38e4f

Saeris added 5 commits November 22, 2023 10:32

Fix file extensions lint errors

5b483f4

Merge branch 'main' into feat/use-interface-for-base-types

a6bbcd1

Add jsdoc @param and @returns required by linting rules

a1584f1

Fix broken export for intersectAsync

7f84338

This is a bugfix for an issue on main

fabian-hiller added the priority This has priority label Nov 29, 2023

Saeris mentioned this pull request Nov 29, 2023

[INTROSPECTION][DISCUSSION] allow to introspect with BaseSchema & BaseSchemaAsync #258

Closed

Saeris added 3 commits December 5, 2023 08:59

Merge branch 'main' into feat/use-interface-for-base-types

cab5616

Fix eslint error

a89b486

Merge branch 'main' into feat/use-interface-for-base-types

3b2d314

fabian-hiller force-pushed the main branch from 0b6f992 to d7404a8 Compare February 26, 2024 07:14

Saeris added 2 commits February 27, 2024 12:37

Add isOfType utility and refactor type guard implementations

fe36ae1

Merge branch 'feat/use-interface-for-base-types' of https://github.co…

9574d5d

…m/Saeris/valibot into feat/use-interface-for-base-types

fabian-hiller force-pushed the main branch 4 times, most recently from b997a0f to 6d0cde4 Compare March 1, 2024 08:22

fabian-hiller requested changes Mar 1, 2024

View reviewed changes

Saeris added 2 commits March 4, 2024 13:28

Merge branch 'main' into feat/use-interface-for-base-types

2a28529

Merge branch 'main' into feat/use-interface-for-base-types

48ce458

run prettier and fix lint warnings

02b17d5

fabian-hiller force-pushed the main branch from b82a1a1 to 55828f5 Compare March 5, 2024 04:27

fabian-hiller requested changes Mar 5, 2024

View reviewed changes

library/src/methods/fallback/fallback.ts Outdated Show resolved Hide resolved

library/src/methods/fallback/fallbackAsync.ts Outdated Show resolved Hide resolved

library/src/methods/getDefault/types.ts Outdated Show resolved Hide resolved

fabian-hiller added 4 commits March 5, 2024 19:13

Add Default and DefaultAsync type and refactor codebase

2cefacc

Add Fallback and FallbackAsync type and refactor codebase

114b1f8

Refactor getDefaults and getFallbacks method

e08af7f

Undo changes of NestedPath type in flatten

03e95e3

fabian-hiller approved these changes Mar 6, 2024

View reviewed changes

fabian-hiller added 3 commits March 6, 2024 00:19

Rename hasType to isOfType and refactor codebase

7e79352

Add Default and Fallback to API reference of website

2f77fc2

Update changelog of library

de1d857

fabian-hiller force-pushed the feat/use-interface-for-base-types branch from c2570d6 to de1d857 Compare March 6, 2024 19:46

fabian-hiller merged commit f6fe5a2 into fabian-hiller:main Mar 6, 2024
6 checks passed

aboqasem reviewed Mar 16, 2024

View reviewed changes

Saeris mentioned this pull request Apr 6, 2024

Rewrite Valibot with the pipe function #502

Merged

Ramyromel mentioned this pull request Jun 27, 2024

[Snyk] Upgrade valibot from 0.28.1 to 0.30.0 Ramyromel/mesc#1

Open

Use interface instead of type for all Base types #259

Use interface instead of type for all Base types #259

Conversation

Saeris commented Nov 19, 2023 • edited Loading

netlify bot commented Nov 19, 2023 • edited Loading

✅ Deploy Preview for valibot ready!

fabian-hiller commented Nov 19, 2023

Saeris left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-hiller commented Nov 22, 2023

fabian-hiller commented Nov 29, 2023

fabian-hiller commented Dec 4, 2023

Saeris commented Dec 4, 2023

fabian-hiller commented Dec 4, 2023

Saeris commented Dec 5, 2023

fabian-hiller commented Dec 5, 2023

fabian-hiller commented Jan 5, 2024

ariskemper commented Jan 5, 2024

Saeris commented Jan 5, 2024

Saeris commented Jan 5, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-hiller commented Mar 4, 2024

Saeris commented Mar 4, 2024

fabian-hiller left a comment

Choose a reason for hiding this comment

fabian-hiller commented Mar 6, 2024

fabian-hiller commented Mar 7, 2024

aboqasem Mar 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabian-hiller Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Use `interface` instead of `type` for all Base types #259

Use `interface` instead of `type` for all Base types #259

Saeris commented Nov 19, 2023 •

edited

Loading

netlify bot commented Nov 19, 2023 •

edited

Loading

aboqasem Mar 16, 2024 •

edited

Loading

fabian-hiller Mar 19, 2024 •

edited

Loading