Skip to content

Conversation

@EskiMojo14
Copy link
Contributor

Currently there's no easy way to get the type for "a schema I can pass to this method", as the Schema type for each is internal. This PR uses type-only namespaces to nest the type under the method/action itself, meaning it avoids conflicts but still allows use if needed.

An example of where this would be useful:

import * as v from 'valibot'

export class TypedHttpClient<T extends v.partial.Schema> {
  constructor(private schema: T) {}

  get(someUrl: string, selectFields: string[]) {
    const rawData = null; // fetch data with includeFields

    const partialSchema = v.partial(this.schema);

    const output = v.parse(partialSchema, rawData);
  }
}

@vercel
Copy link

vercel bot commented Sep 16, 2025

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
valibot Ready Ready Preview Comment Sep 16, 2025 11:14am

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. enhancement New feature or request labels Sep 16, 2025
@EskiMojo14 EskiMojo14 changed the title use namespaces to export "internal" types that are used in exported signatures feat: use namespaces to export "internal" types that are used in exported signatures Sep 16, 2025
BaseSchema<unknown, unknown, BaseIssue<unknown>>,
ErrorMessage<TupleWithRestIssue> | undefined
>;
export namespace args {
Copy link
Contributor

Choose a reason for hiding this comment

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

Cool PR!

If you change these to ambient namespaces it will enforce that no values are added to these namespaces. It will also help with the erasableSyntaxOnly flag

Suggested change
export namespace args {
export declare namespace args {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, I'm still allowed to do export const in an ambient namespace

erasableSyntaxOnly is a good shout, worth checking if anything valibot is doing violates that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, though that was added in TS 5.8 and this repo is currently on 5.7.3

Copy link
Contributor

Choose a reason for hiding this comment

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

I tend use ambient namespaces by default. And if Valibot uses 5.7.3, in userland if an enum or value-level namespace is used, it can't be erased for users that are on later versions of TS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just checked - no source code uses enums, but there are some used in tests for schemas that support native enums

Copy link
Contributor

@ahrjarrett ahrjarrett Sep 16, 2025

Choose a reason for hiding this comment

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

To clarify -- if you write export const x = 1 in an ambient namespace, the namespace and x will be erased at runtime.

If you write export const x = 1 in a regular namespace, then namespace won't be erased, and x will be shipped with the JS bundle

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, but the const is still in the type declarations so as far as typescript is concerned it exists, which seems worse imo

Copy link
Contributor

@ahrjarrett ahrjarrett Sep 16, 2025

Choose a reason for hiding this comment

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

If that's a concern, then I recommend this:

export declare namespace args {
  export type { Schema }
}

type Schema = // ...

That way there's no pattern of adding anything to the namespace itself. It also allows users to access Schema in both places

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think I'd rather leave it as is, and maybe we get a follow up PR for updating TS and enabling erasableSyntaxOnly if that's desired

Copy link
Contributor

@ahrjarrett ahrjarrett Sep 16, 2025

Choose a reason for hiding this comment

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

The diff would be much smaller that way too 🤷 it can only help your chances of getting it merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants