Closed loynoir closed 1 year ago
@loynoir Heya,
https://github.com/evanw/esbuild/issues/3077#issuecomment-1518713419
I wouldn't expect any bundler to try and tree shake at the object level. Also the code example you have there is importing the TypeRegistry
as a default export
(which is not a design implemented by TypeBox). The current namespaces are implemented specifically as inline IIFE structures. They use the keyword namespace
only as an alias for const TypeRegistry = ((exports) => { ... })(exports)
and nothing more. This is intentional.
In future, TypeBox may move each namespace to a ESM module (i.e. in a specific type-registry.mts
module). But for now typebox.ts
is intentionally written to be a singular file and the IIFE helps to ensure proper encapsulation of the Map
they contain within this file. Note, the typebox.ts
file is kept as a singular file for both historical reasons, as well as to make downstream tooling simpler as exports can be obtained via top level exports can be resolved without needing to consider varying module systems / multiple file resolution strategies.
https://github.com/microsoft/TypeScript/issues/30994#issuecomment-492017219
Namespaces are not out of date. They are merely aliases for IIFE's (as mentioned above). But as per quote (with relevant takeaways highlighted in bold)
Namespaces are probably never going away and, simultaneously, you probably shouldn't use them, since better, more standard and modern patterns exist through the use of modules. If you have code that you feel needs to use a namespace, more power to you and go for it,
And the TypeBox project "feels the need" to use them (at least for now)
https://github.com/microsoft/TypeScript/pull/51387 | https://github.com/microsoft/TypeScript/issues/39247
The suggestion here is that performance is improved for statically bound identifiers, where-as IIFE's evaluate return structures at runtime. This is true (as far as I've observed in V8 optimizations). However, the IIFE's being considered (presumably the TypeRegistry, FormatRegistry, TypeResolvers) here are called so infrequently, you're not likely to see any significant performance increase moving these to modules in real codebases (with perhaps a minor bump for the Resolvers which are called through type composition, but even these are hindered by dynamic object creation for the schematics)
The main components targeted for performance is the TypeCompiler
(IIFE) and TypeCheck
(class) types. It should be possible to produce a benchmark where the you move the TypeCompiler
into a ESM module (using statically bound functions), and try and compile a type over a series of iterations. TypeBox measures compilation performance, so if by moving to ESM shows a demonstrable improvement on compilation performance, I may consider refactoring the compiler components to split sub modules.
Here is the current compilation benchmarks using IIFE. Link here
┌────────────────────────────┬────────────┬──────────────┬──────────────┬──────────────┐
│ (index) │ Iterations │ Ajv │ TypeCompiler │ Performance │
├────────────────────────────┼────────────┼──────────────┼──────────────┼──────────────┤
│ Literal_String │ 1000 │ ' 243 ms' │ ' 8 ms' │ ' 30.38 x' │
│ Literal_Number │ 1000 │ ' 195 ms' │ ' 5 ms' │ ' 39.00 x' │
│ Literal_Boolean │ 1000 │ ' 162 ms' │ ' 4 ms' │ ' 40.50 x' │
│ Primitive_Number │ 1000 │ ' 168 ms' │ ' 6 ms' │ ' 28.00 x' │
│ Primitive_String │ 1000 │ ' 164 ms' │ ' 5 ms' │ ' 32.80 x' │
│ Primitive_String_Pattern │ 1000 │ ' 214 ms' │ ' 9 ms' │ ' 23.78 x' │
│ Primitive_Boolean │ 1000 │ ' 132 ms' │ ' 4 ms' │ ' 33.00 x' │
│ Primitive_Null │ 1000 │ ' 148 ms' │ ' 4 ms' │ ' 37.00 x' │
│ Object_Unconstrained │ 1000 │ ' 1158 ms' │ ' 30 ms' │ ' 38.60 x' │
│ Object_Constrained │ 1000 │ ' 1263 ms' │ ' 25 ms' │ ' 50.52 x' │
│ Object_Vector3 │ 1000 │ ' 384 ms' │ ' 7 ms' │ ' 54.86 x' │
│ Object_Box3D │ 1000 │ ' 1932 ms' │ ' 27 ms' │ ' 71.56 x' │
│ Tuple_Primitive │ 1000 │ ' 478 ms' │ ' 14 ms' │ ' 34.14 x' │
│ Tuple_Object │ 1000 │ ' 1232 ms' │ ' 14 ms' │ ' 88.00 x' │
│ Composite_Intersect │ 1000 │ ' 671 ms' │ ' 17 ms' │ ' 39.47 x' │
│ Composite_Union │ 1000 │ ' 537 ms' │ ' 18 ms' │ ' 29.83 x' │
│ Math_Vector4 │ 1000 │ ' 816 ms' │ ' 14 ms' │ ' 58.29 x' │
│ Math_Matrix4 │ 1000 │ ' 417 ms' │ ' 6 ms' │ ' 69.50 x' │
│ Array_Primitive_Number │ 1000 │ ' 378 ms' │ ' 5 ms' │ ' 75.60 x' │
│ Array_Primitive_String │ 1000 │ ' 353 ms' │ ' 6 ms' │ ' 58.83 x' │
│ Array_Primitive_Boolean │ 1000 │ ' 279 ms' │ ' 5 ms' │ ' 55.80 x' │
│ Array_Object_Unconstrained │ 1000 │ ' 1794 ms' │ ' 20 ms' │ ' 89.70 x' │
│ Array_Object_Constrained │ 1000 │ ' 1586 ms' │ ' 19 ms' │ ' 83.47 x' │
│ Array_Tuple_Primitive │ 1000 │ ' 791 ms' │ ' 13 ms' │ ' 60.85 x' │
│ Array_Tuple_Object │ 1000 │ ' 1638 ms' │ ' 17 ms' │ ' 96.35 x' │
│ Array_Composite_Intersect │ 1000 │ ' 796 ms' │ ' 17 ms' │ ' 46.82 x' │
│ Array_Composite_Union │ 1000 │ ' 798 ms' │ ' 15 ms' │ ' 53.20 x' │
│ Array_Math_Vector4 │ 1000 │ ' 1127 ms' │ ' 14 ms' │ ' 80.50 x' │
│ Array_Math_Matrix4 │ 1000 │ ' 677 ms' │ ' 9 ms' │ ' 75.22 x' │
└────────────────────────────┴────────────┴──────────────┴──────────────┴──────────────┘
If you can show a 30% improvement for compilation (or validation) performance, I'll consider a refactor of the compiler components into sub modules (under a different GH issue specific to performance).
While replacing namespaces for modules are going to be better in the long term (and is planned for), this isn't something I'm prepared (or able) to implement at this time. As it stands.
import { T }
specifier as per design.TypeCompiler
into sub modules.Copy and paste the following into a .html
file.
<html>
<script type="module">
import { Type, TypeRegistry } from 'https://esm.sh/@sinclair/typebox@0.28.4'
console.log(Type) // ok
console.log(TypeRegistry) // ok
</script>
</html>
@loynoir Going to close this one out. But would be open to a different issue related to performance if you can demonstrate Value
or TypeCompiler
modules show a performance increase using statically bound identifiers (ESM or otherwise). I may run some tests on this myself when get some time to do so.
If benchmarking your side, the Value
namespaces may be easier to benchmark (specifically the ValueCheck
as there is minimal cross module dependence). It would also be more attune to throttling in performance benchmarks as it doesn't runtime evaluate (so should reveal performance improvements, and a 30% increase would be worth a refactor)
Cheers S
I wouldn't expect any bundler to try and tree shake at the object level.
If tree-shake shake out anything leads to different output, it must be a bug.
Anything after optimization have a runtime difference, I wil use word unglify
or minify
, not word tree-shake
.
tree-shake
is not a monster, but unglify
or minify
maybe.
Also the code example you have there is importing the
TypeRegistry
as adefault export
(which is not a design implemented by TypeBox).
My main goal is to decrease bundle size.
To achive that goal, source code should be written in good style of tree-shake think it is definitely safe.
Thus, at prev feat, I suggest a way have good tree-shake support, using namespace__method
.
But current code style use namespace.method
, I don't remeber dot property like syntax have tree-shake
.
I tested default export
, and found esbuild does not tree-shake ExportDefaultDeclaration
.
And thus, I open a feat in esbuild to tree-shake default export.
That is why
the code example you have there is importing the
TypeRegistry
as adefault export
(which is not a design implemented by TypeBox)
Because I'm thinking about the PR implement
dot property like syntax have tree-shake
for code organizing.And evanw
points out a way dot property like syntax have tree-shake
.
I may implement in PR like below,
if I have time
you are willing to change
==> user.mts <==
import { someFn } from './typebox.mjs'
someFn()
==> typebox.mts <==
import * as TypeRegistry from './TypeRegistry.mjs'
function someFn() {
TypeRegistry.Clear()
}
export { someFn, TypeRegistry }
==> TypeRegistry.mts <==
const map = new Map<string, unknown>()
export function Entries() {
return new Map(map)
}
export function Clear() {
return map.clear()
}
// should not use, `tree-shake export default` is not supported by any bundler
// https://github.com/evanw/esbuild/issues/3077#issuecomment-1518713419
// export default { Entries, Clear }
$ npm exec -- esbuild --format=esm --bundle ./user.mts
// TypeRegistry.mts
var map = /* @__PURE__ */ new Map();
function Clear() {
return map.clear();
}
// typebox.mts
function someFn() {
Clear();
}
// user.mts
someFn();
@sinclairzx81
As typescript have a very large codebase, so can benifit a lot even from small optimization idea.
I don't think typebox can benifit a lot, besides I feel enough about typebox performance, not my main goal.
My main goal is possible bundle size
So, the problem is are you willing to change, if only shows smaller size?
If you are not willing to accept that PR only shows smaller bundle size, then I have no interest to implement such large PR when I have time.
Namespaces are not out of date. They are merely aliases for IIFE's (as mentioned above). But as per quote (with relevant takeaways highlighted in bold)
Namespaces are probably never going away and, simultaneously, you probably shouldn't use them, since better, more standard and modern patterns exist through the use of modules. If you have code that you feel needs to use a namespace, more power to you and go for it,
And the TypeBox project "feels the need" to use them (at least for now)
weswigham
points out
Namespaces are probably never going away and, simultaneously, you probably shouldn't use them, since better, more standard and modern patterns exist through the use of modules. If you have code that you feel needs to use a namespace, more power to you and go for it, but most of the time you don't need one, as we now have a different conceptual organizational model recommended for large-scale JS (modules). That's why @DanielRosenwasser said
namespaces are not a huge loss
- most new TS code should probably never need to use them, and should think hard about if they really need to.Minimally, we'll never outright remove namespaces because they're incredibly useful for describing type hierarchies and the like of existing cjs libraries - it's just that modern
esm
-like code probably doesn't need them, since the module itself is a namespace (where nesting is achieved thru reexports).
evanw points out
import * as TypeRegistry from './lib.mjs'
difference between js object and [module namespace exotic objects](https://tc39.es/ecma262/#sec-module-namespace-exotic-objects)
which make it safe for bundler optimization
I don't think it's worth it for esbuild to add additional complexity to support these unusual and non-recommended workflows. Instead, I recommend that you do code organization using normal ESM exports the way they are intended to be used.
most of the time you don't need namespace
module is the recommended for code organizing.
new ts code should probably never need to use namespace
think hard about if really need to use namespace
namespace are not deprecated because of CJS
typebox is already esm-like, using import
, not require
namespace XXX {}
is not getting any safe bundler size optimization now and might never will.
That is why
namespace XXX {}
as featThere is no reason to assume TypeScript plans to deprecate namespaces
weswigham
points out
Minimally, we'll never outright remove namespaces because they're incredibly useful for describing type hierarchies and the like of existing cjs libraries
If you can show a 30% improvement for compilation (or validation) performance, I'll consider a refactor of the compiler components into sub modules (under a different GH issue specific to performance).
Again, I feel enough about current typebox performance, thus I lack to interest.
But I feel not enough about bundle size.
@loynoir Heya
My main goal is to decrease bundle size. To achive that goal, source code should be written in good style of tree-shake think it is definitely safe.
In TypeBox, there has be a lot of care and attention given to ensuring module cross dependence is only taken when necessary. In this regard, TypeBox doesn't rely on tree-shaking, nor does it assume tree-shaking is even available in a users toolchain. Instead it is specifically crafted such that bundle sizes are proportional to the modules (or sub modules) a user directly imports, with each module considered to be an "un-shakable" unit due to cross coupling for things internal to any given module. This is a designed for, it's not an oversight.
Of course, there is always ways to improve things. For example, currently the Value
sub module is very large, and it may be beneficial to support sub module imports for @sinclair/typebox/value/check
, @sinclair/typebox/value/delta
(in the short term), this would be inline with the modular design of TypeBox (and would let users have explicit control over bundle output sizes) without relying on tree shakers to operate as expected.
And evanw points out a way dot property like syntax have tree-shake.
Unfortunately, I can't accept a PR that splits the typebox.ts
module into sub modules at this time. As noted, there are reasons why this module is specifically authored as a single module. Splitting the module now and breaking downstream users just to shave off a few kb is not a viable trade off (it's too much of a breaking change). This module will be sub moduled closer to a 1.0 release, under the @sinclair/typebox/type/Type.js
and @sinclair/typebox/type/TypeRegistry.js
paths respectively.
typebox is already esm-like, using import, not require
This isn't a coincidence. TypeBox will become fully ESM closer to a 1.0 release, which each sub module split at the namespace level. This is planned for with the namespaces denoting where the module should be split (but not at this time)
Again, I feel enough about current typebox performance, thus I lack to interest. But I feel not enough about bundle size.
I am very mindful of keeping bundle sizes down, but at this stage. It's just not a high priority for the project. For example this is the current breakdown of TB bundle sizes compared to the similar libraries. https://bundlephobia.com/package/@sinclair/typebox@0.28.4
The focus at this stage to keep bundle sizes down through using algorithm techniques, not through sub modules and reliance on tree shakers (those are bonus once TB finally transitions to ESM)
I can't accept a PR that splits the
typebox.ts
module into sub modules at this time.
Oh, I understand you.
You mean you don't want current typebox.ts
have any import statement
, right?
Beside, typebox.ts
might not the only thing need to change.
TypeRegistry
is the thing I found not tree-shake-able, or said bundler thinks it is not safe to do tree-shake, at my first glance at output.
These aren't tree shake-able (afaik) (as all functionality may be invoked at runtime depending on the schema).
In my proof PR, I do a simple two commit, got 12.7% smaller size.
Bundle size decrease from 148.7 to 129.8.
Said about namespace XXX {}
, I found at least one unused method, typebox internally and test bundle source internally.
I feel the size can be better than 129.8, because namespace XXX {}
is mutable thus not safe for optimization
Besides, I feel not enough about bundle size, but still acceptable.
Is there a estimate time when typebox will move away from namespace XXX {}
? @sinclairzx81
TypeRegistry is the thing I found not tree-shake-able, or said bundler thinks it is not safe to do tree-shake, at my first glance at output.
The TypeRegistry actually has cross dependence with the TypeGuard as it's used to verify a type is registered prior to compilation. Also as the TypeGuard is used extensively during type composition, so both are critical components of typebox.ts
Is there a estimate time when typebox will move away from namespace XXX {}?
I'm not sure at this stage, but the following is a very high level (and tentative) series of steps that need to happen to remove them.
1) Investigate ESM support WITH namespaces (and whatever file / transforms are required to support it) 2) Refactor Value sub modules (remove namespaces, use default export) 3) Refactor Compiler modules (remove namespaces. use default export) 4) Refactor Type (split to sub modules, use default export) 5) Provision 1.0 release
Each step is a minor revision in and of itself, leading towards the following import scheme.
import TypeCompiler from 'typebox/compiler'
import Value from 'typebox/value'
import Type, { Static } from 'typebox'
And optionally
import { ValueCheck } from 'typebox/value'
Initial estimates for namespace removal are likely 4th quarter 2023 (all things going well) or early 2024. Until then, priority is going into achieving all envisioning functional requirements while preserving the current namespace setup (as it has generally provided me most agility and flexibility to evolve the project over the years without breaking users or being locked into a finalized API design).
Hope this helps!
Sorry for forget to comment out export default
.
const map = new Map<string, unknown>()
export function Entries() {
return new Map(map)
}
export function Clear() {
return map.clear()
}
// should not use, `tree-shake export default` is not supported by any bundler
// https://github.com/evanw/esbuild/issues/3077#issuecomment-1518713419
// export default { Entries, Clear }
import * as TypeRegistry from './TypeRegistry.mjs'
function someFn() {
TypeRegistry.Clear()
}
export { someFn, TypeRegistry }
Quote evanw
It looks like Rollup and Parcel don't do this transformation either, which is presumably due to the same complexity issues. I recommend that you use normal ESM exports the way they are intended to be used if you are interested in enabling tree shaking of your code:
import * as TypeRegistry from './lib.mjs'
Description
https://github.com/evanw/esbuild/issues/3077#issuecomment-1518713419
Evanw suggest syntax of
import * as TypeRegistry from './lib.mjs'
.Which have is a dot property syntax, like namespace, with tree shake support.
https://github.com/evanw/esbuild/issues/3077#issuecomment-1518725003
https://github.com/microsoft/TypeScript/issues/30994#issuecomment-492017219
https://github.com/microsoft/TypeScript/issues/39247
https://github.com/microsoft/TypeScript/pull/51387
Related
https://github.com/sinclairzx81/typebox/issues/401
https://github.com/sinclairzx81/typebox/issues/399