sinclairzx81 / typebox

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

Indicate which files don't have "sideEffects" in package.json for improved tree shaking #907

Open CraigMacomber opened 3 weeks ago

CraigMacomber commented 3 weeks ago

I think (but have not confirmed) that part of the reason I'm seeing more TypeBox code get included in my bundle than expected is because TypeBox does not specify "sideEffects": false in its package.json file.

Assuming TypeBox does not rely on import side-effects, including that should improve tree shaking in webpack based bundles, see https://webpack.js.org/guides/tree-shaking/

sinclairzx81 commented 3 weeks ago

@CraigMacomber Hi,

Assuming TypeBox does not rely on import side-effects, including that should improve tree shaking in webpack based bundles, see https://webpack.js.org/guides/tree-shaking/

TypeBox does actually have import side effects with the TypeRegistry, FormatRegistry modules. These modules keep a common map to store user registrations, and have been somewhat awkward in the past with respect to bundler behaviours (I recently had some issues with esm.sh importing multiple (non-singleton) instances of these modules)

https://github.com/sinclairzx81/typebox/blob/master/src/type/registry/type.ts

https://github.com/sinclairzx81/typebox/blob/master/src/type/registry/format.ts

These modules are imported specifically by the /compiler and /value and /error submodules. I'm not sure there's a better way to describe them in configuration that would help bundlers tree shake (but I'm not an expert in this area). There may be something that can be done, but I expect these registries may need to be turned into class instances or similar (which would be a breaking API change), but maybe there are ways to configure for them?

Are you able to advise on a better configuration?

CraigMacomber commented 3 weeks ago

(I recently had some issues with esm.sh importing multiple (non-singleton) instances of these modules)

I think that's actually an unrelated (and pretty common) issue.

Requiring that all users use the same copy of a module is a different requirement than sideEffects:false, which implies that if there are no usages of the items exported from the module, its safe to skip importing it.

"sideEffects" can take an array of files which actually have side-effects so that might be usable here if we really have required side effects in some modules.

Looking at those two files though, I don't think they have "sideEffects". The kind of thing that would count as a side effect would be some module adding its own types to a registry in a different module when it is imported (ex: importing one module causes it to call TypeRegistry.Set). If there are any such modules (maybe default sets of types?), you could list them in "sideEffects".

Looking at all usages of TypeRegistry.Set, none seem to be in actual production code at the top level of a module, so I don't see any sideEffects there, but maybe I missed something, or the other registry has some.

I suspect its common for apps using TypeBox to have side effects due to their use or type box and its registries, but perhaps TypeBox itself does not have any sideEffects?

Also its worth noting that we actually only care about sideEffects where removing them via treeShaking would be a problem. For example if a module creates a type and adds it to the TypeRegestry, it might still be ok to skip importing that module in the case where that module is unused (and thus the type it registered is unused) since the app may not care its in the type registry in that case. Since it appears you allow iterating the TypeRegistry, such a choice would likely be observable, so maybe its not a good idea in general, but it might be ok for some apps (and this impacts if they can say their code using TypeBox has no SideEffects, not if TypeBox it self has any)

Edit: I should note this is an area I'm actively learning and not an expert in. I think the above is accurate, but you may want to confirm it yourself.

marcesengel commented 3 weeks ago

Ironic, I just created a PR for this 😂. See #909.

I suspect its common for apps using TypeBox to have side effects due to their use or type box and its registries, but perhaps TypeBox itself does not have any sideEffects?

This. Import of the files itself doesn't cause sideEffects, actually using the exports does/can. That's a separate issue and will work just as expected, because by default bundlers consider all functions to have side effects (even when importing from a package with sideEffects: false in the package.json).

Because of that, adding the field won't change anything all together, unless you also use the the /*#__PURE__*/ annotation on all top-level invocations of TypeBox functions in your file or you don't invoke any exported functions.