Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
CraigMacomber opened this issue Jun 17, 2024 · 3 comments

Comments

@CraigMacomber
Copy link

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
Copy link
Owner

@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
Copy link
Author

CraigMacomber commented Jun 17, 2024

(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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants