sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.77k stars 152 forks source link

Individual types imports naming conflict with native classes #747

Closed cyrilletuzi closed 7 months ago

cyrilletuzi commented 7 months ago

Hello,

First, a big thank you for this amazing library, which solved a major pain point I had in one of my own library from several years. ♥️

Version 0.32 with ESM support and individual imports is just what I needed too. 👏🏻

But by using it in real world scenarios, I quickly ran into a problem. Doing this:

import { Array, Boolean, Number, Object, Optional, String } from '@sinclair/typebox';

is very problematic, because nearly all of these already exist in native JavaScript. Thus, by doing this, it is overriding JavaScript native classes. And so if doing something like Array.from() in the file, it does not work anymore. And I can easily imagine more invisible side effects to this.

So I ended up aliasing every imports:

import { Array as ArrayType, Boolean as BooleanType, Number as NumberType, Object as ObjectType, Optional as OptionalType, String as StringType } from '@sinclair/typebox';

It fixes the issue, but is really inconvenient.

Would you consider a different naming convention for all these exports (it does not matter the convention, as long as it does not conflict with native classes)?

I suppose you already know this, but just in case, changing the naming would not require to change the whole project naming, just the exports in the entry point, like that:

export { Array as ArrayType } from './internal';

I can take care of the Pull Request, if wanted and needed.

Thank you.

sinclairzx81 commented 7 months ago

@cyrilletuzi Hi,

Thanks for the suggestion and offer to update things here. While I do think overlapping names is valid concern, aliasing the type names does offer a viable work around (as does namespacing via import * as Type). So somewhat keen to keep on with the current exported names (as they do not break other import patterns)

Future revisions of Typebox may provide the following import schemes.

import Type, { type Static } from '@sinclair/typebox' // Type.String (not Type.StringType)

import * as Type from '@sinclair/typebox' // Type.String (not Type.StringType)

import { Type } from '@sinclair/typebox'

import { String } from '@sinclair/typebox'

So based on the above imports, I'm somewhat reserved about changing the names of the exported functions, if only to keep the API surface consistent.

Hope this brings some insight Cheers S

cyrilletuzi commented 7 months ago

@sinclairzx81 Thank you for your quick answer.

But I am not sure to understand it:

import { String } from '@sinclair/typebox'

Is it not already what we can do in v0.32 and what I put in my example?

import Type, { type Static } from '@sinclair/typebox' // Type.String (not Type.StringType) import * as Type from '@sinclair/typebox' // Type.String (not Type.StringType) import { Type } from '@sinclair/typebox'

I think these solutions mean losing the ESM advantage of tree-shaking right? My issue is in the context of individual imports. I put nearly all the types in my example to show all the possible conflicts with existing JavaScript native classes, but I would like to be able to import only what I use.

I could also think of another solution: it is possible to export something twice. Something like:

export { Array, Array as ArrayType } from './internal';

In the end it is your choice, but as a library author myself I would warn you about the risks of this naming:

sinclairzx81 commented 7 months ago

@cyrilletuzi Hi, thanks for the follow up.

I think these solutions mean losing the ESM advantage of tree-shaking right? My issue is in the context of individual imports. I put nearly all the types in my example to show all the possible conflicts with existing JavaScript native classes, but I would like to be able to import only what I use.

Yeah, it would likely mean losing tree-shaking if using * barrel imports specifically, but this is largely a consequence of current bundlers not providing options to aggressively optimize output bundles. Note that with compilers like TypeScript (or future bundler technology), it is plausible that one day, they may be able to omit unused imports (by statically deriving specific imports from usage). Unfortunately this isn't a thing today, so we're in a situation where developers need to be specific about what they import....which isn't ideal imo.

TypeBox 0.32.0 did add the option to for individualized imports (which was a concession to provide some library level options to manually optimize output bundle (at a non-zero maintenance cost to the library)), but the overall preference is to retain the Type.* as the primary API (while continuing to hold out for more capable bundling tools), with selective imports used to manually optimize bundles (if bundle sizes are a concern)

I could also think of another solution: it is possible to export something twice. Something like:

I think import aliases would be better here than if TypeBox exposes duplicate type names.

using names already existing in native JavaScript seems risky by itself, I can easily imagine other conflict issues (like conflicting typings...)

I do partially agree, however TypeScript is capable of statically detecting usage issues (i.e. if a user attempts to call Array.from() on a TypeBox type). In these cases, prefixing globals with globalThis or just using the import alias is a viable workaround (which TypeBox does internally). Of course, this isn't really ideal, but it is something that can be trivially worked around.

Note that in other languages, importing full prefix namespaces is the ideal way to avoid naming conflicts, but as mentioned previously, JavaScript just isn't there with it's bundling technologies, and it's unfortunate that "naming conflicts" and "bundlers" (which are two completely orthogonal things) are at odds with one another.


So, I do understand the concern here, but at this point in time, the preference is to retain the current API for selective imports (as it generally keeps things consistent and inline with future plans for the library). I am also holding out for future advancements in bundler technologies that would support tree-shaking for Type.* and import * as Type usage patterns without having to explicitly import each type individually (something I had been considering exploring myself)

Hope this brings a bit more insight into the reasoning here.

cyrilletuzi commented 7 months ago

Thank you for the detailed explanation.

overall preference is to retain the Type.* as the primary API

Noted, I will keep this form in the documentation of my library where there are typebox examples then.