-
-
Couldn't load subscription status.
- Fork 281
feat: use namespaces to export "internal" types that are used in exported signatures #1310
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
892befe to
e50bbd8
Compare
| BaseSchema<unknown, unknown, BaseIssue<unknown>>, | ||
| ErrorMessage<TupleWithRestIssue> | undefined | ||
| >; | ||
| export namespace args { |
There was a problem hiding this comment.
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
| export namespace args { | |
| export declare namespace args { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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: