sinclairzx81 / typebox

Json Schema Type Builder with Static Type Resolution for TypeScript
Other
4.56k stars 148 forks source link

Types for both CJS and ESM cause ambiguity and prevent correct type resolution. #890

Closed akim-bow closed 3 weeks ago

akim-bow commented 1 month ago

I have NestJS in my project which uses nestjs-typebox package.

I use ESM in my project and as a result, when i do something like this import { type Static, Type, type TSchema } from '@sinclair/typebox' I receive ESM types.

However, nestjs-typebox package relies on CJS imports and its API is based on CJS types from '@sinclair/typebox. Turns out that ESM and CJS types aren't compatible and cannot be used interchangeably.

For example, this code fails in typescript (i use 5.4.5).

// File has `type: module`, but here i explicitly import types as if it's CJS
import type { Static, TSchema } from '@sinclair/typebox' with { 'resolution-mode': 'require' };
// Regular ESM import
import { Type } from '@sinclair/typebox';

// ERROR: Property [Kind] is missing in type TString but required in type TSchema
const type: TSchema = Type.String();

This is because these types aren't compatible.

I suggest to output just a single type folder to avoid this confusion.

sinclairzx81 commented 1 month ago

@akim-bow Hi,

Turns out that ESM and CJS types aren't compatible and cannot be used interchangeably.

Yes, this was a known issue when TypeBox added ESM support. The initial recommendation was that if a sub dependency was based on CJS (in your case nestjs-typebox) then downstream implementations should also limit their builds to CJS (ensuring the same types are pulled in), but this was just the initial recommendation (as of 6 months ago)

I suggest to output just a single type folder to avoid this confusion.

I'm not sure I follow. TypeBox provides dual publishing for CJS and ESM which it achieves by publishing separate builds for each. Maybe there is a better way to achieve this? Ideally I would prefer to opt for ESM only (if single publishing), however limiting to ESM only would break many projects who already rely on CJS (and for historical reasons, tooling support for CJS still the most understood, meaning publishing for CJS only is likely still the most viable target for single publishing).

As far as I can tell, there does not appear to be an ideal solution for mixing and matching ESM/CJS that doesn't result in configuration pushback in user applications. But am open to reviewing the current build setup and changing it if there are better solutions available today.

Do you have any thoughts on a possible better setup?

akim-bow commented 1 month ago

@sinclairzx81 Hey, thanks for the detailed response. In order to make myself clear, i've opened PR with my idea. As far as i understand, having just a single type folder (but still keeping javascript files in dual build format) will solve this issue and won't break other projects bcs source files will be same and types will be CJS only, but i am almost certain that TS doesn't care which module system should be used for type resolution. Please correct me if i'm wrong.

akim-bow commented 1 month ago

However your utility for checking package integrity reports this issue.

akim-bow commented 1 month ago

There are 2 consequences following this decision listed in the link above. Probably we can check whether it's the case in the library and if so, deal with them. Removing the necessity to switch to other module system due to downstream dependency seems like a great feat to me.

sinclairzx81 commented 1 month ago

@akim-bow Hi, thanks for the PR

There are 2 consequences following this decision listed in the link above. Probably we can check whether it's the case in the library and if so, deal with them. Removing the necessity to switch to other module system due to downstream dependency seems like a great feat to me.

Yes, I see what you mean by the "single types directory". This approach was actually attempted when TypeBox introduced ESM. However the "Masquerading as CJS" error would be a problem as it has a potential to break type checking in projects (which is a no go zone). The current recommendation (afaik) would be that there needs to be one set of definitions per build target (so one for CJS and one for ESM) as TypeScript will interpret each target slightly differently.


Note that the strategy TypeBox selected was largely informed by the work of the author of are-the-types-wrong (github link here) where he had done the research to verify each publishable target. TypeBox settled on the package-json-redirects strategy as it provided legacy support for Node10 (explicit sub directory resolution) as well as more modern Node resolution schemes.

Other strategies are:

So all this said, I would be open to exploring alternative builds, just so long as they aligned with attw check and satisfied the following criteria.

Requirements:

This would need researching. Unfortunately the documentation for dual publishing is very light, but there may be potential to opt for one of the other build targets. It just needs looking into and ensuring the target meets the above criteria.

akim-bow commented 1 month ago

Yes, Masquerading as CJS is serious. But my question was: is it possible to restructure and rewrite the library's code in a way, that will alleviate any runtime errors? Also edge cases, when "masquerading" leads to errors, could be covered with proper tests to prevent possibility of the errors in the future.

Also i want to underline that i'm totally OK with the approach package-json-redirects. I'm just trying to change type resolution strategy still enjoying the benefits of currently settled strategy.

So all this said, I would be open to exploring alternative builds, just so long as they aligned with attw check and satisfied the following criteria.

Attw checks not really perfect bcs they don't actually analyze your codebase to say whether it will give runtime error or not. It reviews package.json files and does report a possibility for the error. It is strange that attw doesn't consider single type folder approach as a viable alternative. It is a real cases for thousands of projects. If it is crucial for your to stay in attw limits, i would like to see some motivation, just curious :)

A good example of the approach i want to achieve - @nestjs/common package. If you look at the package files, it uses single type emission. And no user has a problem with that structure judjing by open issues there.

To me, forcing user to switch to different module system, forbidding him using ESM packages is a tremendous drawback. As you can see in the top message, i wasn't doing something magical, merely tried to add your wonderful library in NestJS as validator. But probably you have other opinion regarding the seriousness of this matter?

akim-bow commented 1 month ago

I believe it is possible to make your code 100% workable in single type folder approach and eager to make some researches. Just want to make sure i don't break any of the project's principle or walking the same path twice.

sinclairzx81 commented 3 weeks ago

@akim-bow Hi,

Hey, have closed off the PR for now as the single type directory needs additional research to go ahead. Will move this issue into discussions for now as it would be good to review the current build, but as as noted, would want to be working inline with TS and Node module resolution strategies (as working against the grain here would likely break a lot of users)

Let's keep this discussion going, as well as see if it is possible to get some additional insights from the community on this. Module systems are far too complex!