feat: use namespaces to export "internal" types that are used in exported signatures#1310
feat: use namespaces to export "internal" types that are used in exported signatures#1310EskiMojo14 wants to merge 1 commit intoopen-circle:mainfrom
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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: