search

sinclairzx81 / typebox

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

ESNext target #900

Closed CraigMacomber closed 3 weeks ago

CraigMacomber commented 3 weeks ago

I work on https://github.com/microsoft/FluidFramework, a happy user of TypeBox, and was working on our JS runtime compatibility story when I noticed TypeBox has its target set to ESNext in the tsconfig and in task/build/esm/compile.ts (Changed in https://github.com/sinclairzx81/typebox/pull/689/files#diff-b55cdbef4907b7045f32cc5360d48d262cca5f94062e353089f189f4460039e0L4 )

I believe setting the module to ESNext was a good choice, but when that was done "target" was changed as well, which I think should be avoided. The TypeScript documentation on the subject ( https://www.typescriptlang.org/tsconfig/#target ):

The special ESNext value refers to the highest version your version of TypeScript supports. This setting should be used with caution, since it doesn’t mean the same thing between different TypeScript versions and can make upgrades less predictable.

For compatibility purposes in applications using TypeBox, it would be nice if TypeBox ensured it did not use extremely new JS language and library features. Picking the oldest "es" target that does not regress TypeBox would be a way to programmatically ensure this.

I tested TypeBox with the target set to es2020 (Which would be idea for me as thats the oldest I care about supporting), and confirmed with the benchmark script that the bundle size did not regress. I got the below both before and after the change:

┌──────────────────────┬────────────┬────────────┬─────────────┐
│       (index)        │  Compiled  │  Minified  │ Compression │
├──────────────────────┼────────────┼────────────┼─────────────┤
│ typebox/compiler     │ '118.3 kb' │ ' 52.1 kb' │  '2.27 x'   │
│ typebox/errors       │ ' 46.2 kb' │ ' 20.9 kb' │  '2.21 x'   │
│ typebox/system       │ '  4.7 kb' │ '  2.0 kb' │  '2.32 x'   │
│ typebox/value        │ '154.9 kb' │ ' 65.5 kb' │  '2.37 x'   │
│ typebox              │ ' 95.7 kb' │ ' 39.8 kb' │  '2.40 x'   │
└──────────────────────┴────────────┴────────────┴─────────────┘

This makes me suspect (but does not confirm) setting a "target" to "ES2020" in tsconfig.json and task/build/esm/compile.ts should be fine.

In accordance with contribution guidelines in the readme I'm filing this as an issue and not a PR, but if you would like a PR making these changes I can provide one.

sinclairzx81 commented 3 weeks ago

@CraigMacomber Hi!

I believe setting the module to ESNext was a good choice, but when that was done "target" was changed as well, which I think should be avoided. The TypeScript documentation on the subject...

Hey, thanks for spotting this, and I agree! There were many updates done to prepare TypeBox for dual publishing CJS/ESM on the 0.32.x PR, and it appears the the ESNext target for ESM was an oversight. Would be keen to fix the target to ES2020 (inline with the current CJS target).

In accordance with contribution guidelines in the readme I'm filing this as an issue and not a PR, but if you would like a PR making these changes I can provide one.

If you would like to submit a PR to help fix this up, that would be really appreciated :)

Many thanks! S

sinclairzx81 commented 3 weeks ago

@CraigMacomber Hi,

Have gone ahead and fixed this up and published a revision on 0.32.33 which is using the ES2020 target for ESM. Figured I'd resolve this as I'm trying to catch up on backlog of issues (currently working different 5 projects atm) :D

FluidFramework

I work on https://github.com/microsoft/FluidFramework, a happy user of TypeBox, and was working on our JS runtime compatibility story when I noticed TypeBox has its target set to ESNext

I actually meant to leave you a follow up comment the other week when you submitted this https://github.com/sinclairzx81/typebox/issues/865#issuecomment-2103464444. I'm actually a fan of FluidFramework, and had noticed the project was using TB quite extensively. I'd paid attention to a couple of notes on the project around bundling size, and partially implemented isolated type imports (added on 0.32.x) as a possible means to help reduce bundling sizes (at a cost of having to import types individually). These may be worth checking out if you've not seen this before.

As an aside, I'm actually very open to receiving feedback from users who really push TypeBox far (which FluidFramework certainly does), this mostly to help identify any pain points when using the type system at scale (hundreds of types). All suggestions and feedback are taken onboard for future revisions, so if you are able to provide any insights, suggestions or general feedback it would really help the project along as well as give focus to new features.

Will close up this issue for now All the best! S

CraigMacomber commented 3 weeks ago

I actually meant to leave you a follow up comment the other week when you submitted this #865 (comment). I'm actually a fan of FluidFramework, and had noticed the project was using TB quite extensively. I'd paid attention to a couple of notes on the project around bundling size, and partially implemented isolated type imports (added on 0.32.x) as a possible means to help reduce bundling sizes (at a cost of having to import types individually). These may be worth checking out if you've not seen this before.

I had not seen that. We use most of the types, so I'm not sure if it will help our case, but its good to know its an option when I get time to look into our bundles size more.

As an aside, I'm actually very open to receiving feedback from users who really push TypeBox far (which FluidFramework certainly does), this mostly to help identify any pain points when using the type system at scale (hundreds of types). All suggestions and feedback are taken onboard for future revisions, so if you are able to provide any insights, suggestions or general feedback it would really help the project along as well as give focus to new features.

Once we are done getting our 2.0 release ready (hopefully this week), we should have time to work on things like bundle size. For now though I have filed https://github.com/sinclairzx81/typebox/issues/907 which I think is the main cause of unexpected bundling (other than some things on our end we need to clean up in our usage of TypeBox accidently pulling in the compiler when we shouldn't).