-
Notifications
You must be signed in to change notification settings - Fork 152
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
Comments
@CraigMacomber Hi,
TypeBox does actually have import side effects with the TypeRegistry, FormatRegistry modules. These modules keep a common 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 Are you able to advise on a better configuration? |
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.
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 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. |
Ironic, I just created a PR for this 😂. See #909.
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 Because of that, adding the field won't change anything all together, unless you also use the the |
@CraigMacomber Hi! Just an update on this.
So, I still think TypeBox may have side effects here. Mostly it comes down to the potential for modules to mutate state on import. For example, consider the following which is registering a format in the global scope. This scope is evaluated on import, meaning the interior state of the registry can change on import. // ----------------------------------------------
// dependency.ts
// ----------------------------------------------
import { FormatRegistry } from '@sinclair/typebox'
FormatRegistry.Set('email', value => IsEmail(value)) // This code is executed on module resolution
export function X() {}
// ----------------------------------------------
// main.ts
// ----------------------------------------------
import { FormatRegistry } from '@sinclair/typebox'
import { X } from './dependency.ts' // importing X causes registry values to change.
const exists = FormatRegistry.Has('email') // is this true or false? In the majority of cases, the The esm.sh issueThe specific esm.sh issue was caused by the following bundling strategy. https://github.com/esm-dev/esm.sh?tab=readme-ov-file#bundling-strategy
So, considering a typical project import tree like the following...
The issue had been that esm.sh assumed no side effects meant it was ok to duplicate some modules, resulting in the following code split.
The consequence of this meant that modules @CraigMacomber @marcesengel So, the above mostly outlines my concerns with just marking TypeBox as side effect free. I think in most setups, setting side effect free is probably not going to cause downstream problems, but given esm.sh issue (which I think bundles via SWC (don't quote me)) there is still a possibility for a bundler to interpret no side effects as meaning it's safe to split and duplicate (as was the case for esm.sh)
I am still keen to explore this further. I'll need to find some documentation on sideEffect array paths though. Are there any examples / prior art of projects selectively marking some internal modules as having side effects? Also, if TypeBox marks internal modules as having side effects, I would need some help testing those configurations (help needed on this aspect) I have temporarily closed off #909 for the time being (general project house-keeping), but still very open to getting a resolution to this issue. PR's are most welcome, but would be good to check the behaviors of popular bundlers (esbuild, webpack) first before going ahead with the configurations. Cheers All |
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/
The text was updated successfully, but these errors were encountered: