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 · 4 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.

@sinclairzx81
Copy link
Owner

@CraigMacomber Hi! Just an update on this.

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?

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 exists variable will be true. However there are cases where this may not be true. As noted on #909 (comment), there has been instances where bundlers (specifically esm.sh) choose to code split module bundles as an optimization where it assumed ESM implied no side effects as a default. It's a bit complicated, let me try and explain.

The esm.sh issue

The specific esm.sh issue was caused by the following bundling strategy.

https://github.com/esm-dev/esm.sh?tab=readme-ov-file#bundling-strategy

By default, esm.sh bundles sub-modules that ain't declared in the exports field. Bundling sub-modules can reduce the number of network requests for performance. However, it may bundle shared modules repeatedly. In extreme case, it may break the side effects of the package, or change the import.meta.url semantics. To avoid this, you can add ?bundle=false to disable the default bundling behavior:

So, considering a typical project import tree like the following...

┌────────────────────────────────────────────────────┐
│                                                    │
│         ┌─────────────────────────────┐            │
│         │    Internal: TypeCompiler   │            │
│         └─────────────────────────────┘            │
│                      |                             │
│                      |                             │
│            ┌──────────────────────┐                │
│            │    FormatRegistry    │                │
│            └──────────────────────┘                │
│                /               \                   │
│               /                 \                  │
│              /                   \                 │
│ ┌──────────────────────┐  ┌──────────────────────┐ │
│ │         a.ts         │  │         b.ts         │ │
│ └──────────────────────┘  └──────────────────────┘ │
│                                                    │
└────────────────────────────────────────────────────┘

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.

┌──────────────────[Bundle A]────────────────────────┐
│                                                    │
│        ┌─────────────────────────────┐             │
│        │    Internal: TypeCompiler   │             │
│        └─────────────────────────────┘             │
│                       |                            │
│                       |                            │
│           ┌──────────────────────┐                 │
│           │    FormatRegistry    │  <- Duplicated As Bundler Assumed No Side Effects
│           └──────────────────────┘                 │
│                                                    │
└────────────────────────────────────────────────────┘
	               |
	               |
┌──────────────────[Bundle B]────────────────────────┐
│                                                    │
│           ┌──────────────────────┐                 │
│           │    FormatRegistry    │  <- Registered Formats here are not observed by the TypeCompiler
│           └──────────────────────┘                 │
│              /               \                     │
│             /                 \                    │
│            /                   \                   │
│ ┌──────────────────────┐  ┌──────────────────────┐ │
│ │         a.ts         │  │         b.ts         │ │
│ └──────────────────────┘  └──────────────────────┘ │
│                                                    │
└────────────────────────────────────────────────────┘

The consequence of this meant that modules a and b where able to register formats, but they were registering to a registry that was different to the one bundled with the TypeCompiler (leading to unknown format errors). This issue took quite a while to figure out and resolve.


@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)

"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.

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
S

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

No branches or pull requests

3 participants