sinclairzx81 / typebox

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

fix: typebox imports not being tree shaken #909

Open marcesengel opened 3 weeks ago

marcesengel commented 3 weeks ago

Current build tooling will not purge unused typebox imports, as it can't be sure the import is pure (free of side effects). This can be solved by adding the sideEffects: false field to the package.json.

For example importing SOME_CONSTANT from

import { Type } from '@sinclairzx81/typebox'

export const MyUnusedSchema = /*#__PURE__*/
  Type.String()

export const SOME_CONSTANT = 1

will currently result in output similar to

var import_typebox = require("@sinclair/typebox");
// no further usage of import_typebox

var SOME_CONSTANT = 1

as the package import is not denoted to be free of side effects. With the proposed change, it would instead result in

var SOME_CONSTANT = 1

allowing to completely exclude TypeBox from the bundle. For more information see https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free.

sinclairzx81 commented 2 weeks ago

@marcesengel Hi, thanks for this and sorry for the delay here, have been very busy of late (I need to catch up on some of these issues!!).

Side Effects

So, first things first, I actually need to figure out if TypeBox is genuinely side effect free, I'm not sure it is as the internal Map kept for the registries can add and remove entries, and subsequent modules importing the registries may see varying entries. Because importing a Registry may result in the caller being "uncertain" of the entries, that can loosely be interpreted as registries having side-effects (where the side effect was caused by other modules registering entries unseen to the importer).

// ---------------------------------------------------------------------
// module A
// ---------------------------------------------------------------------
import { TypeRegistry } from '@sinclair/typebox'

TypeRegistry.Set('Foo', ...) // adds an entry

// ---------------------------------------------------------------------
// module B
// ---------------------------------------------------------------------
import { TypeRegistry } from '@sinclair/typebox'

// The question is, can module B be sure Foo is registered? The answer is likely
// "it depends on the import resolution order", where the resolution order and
// registration can be viewed as a side-effect.

TypeRegistry.Has('Foo') // true | false ?

So, for this PR to merge, it needs to be determined if the above fits the criteria of 'side-effect'. If it doesn't then we can go ahead with this PR, otherwise may need to defer.

Package.Json

Just on the PR updates. TypeBox actually constructs the publishable package.json through a build task. If possible, can you remove the "sideEffects": false, from the root package.json, and just append to the following.

https://github.com/sinclairzx81/typebox/blob/master/task/build/package/create-package-json.ts#L81-L100

function resolveMetadata() {
  const packagePath = Path.join(process.cwd(), 'package.json')
  const packageJson = JSON.parse(Fs.readFileSync(packagePath, 'utf-8'))
  return {
    name: packageJson.name,
    version: packageJson.version,
    description: packageJson.description,
    keywords: packageJson.keywords,
    author: packageJson.author,
    license: packageJson.license,
    repository: packageJson.repository,
    // flagged by socket.dev if not present
    scripts: { test: 'echo test' },
    // disable auto bundle strategy: see https://github.com/esm-dev/esm.sh#bundling-strategy
    'esm.sh': { 'bundle': false }, 
    // side-effect hint for bundlers (WebPack)
    'sideEffects': false, // added
    types: "./build/cjs/index.d.ts",
    main: "./build/cjs/index.js",
    module: "./build/esm/index.mjs"
  }
}

Thanks again!

marcesengel commented 2 weeks ago

Hi @sinclairzx81,

thanks for the response! I'll move it, no worries 🙌

Summary

The package.json fields only indicates that the importing itself is free of side-effects not the imported functions!

Explanation

I addressed this concern in the ticket, quoting from https://github.com/sinclairzx81/typebox/issues/907#issuecomment-2176054685:

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.

Examples

Import Only

import { TypeRegistry } from '@sinclair/typebox'
// eof

Would be tree-shaken with sideEffects: false, because the import is unused and the import is marked as free of side-effects. If, however, sideEffects is not set, the import would be kept, because the bundler can not be sure that there are no side-effects like for example applying polyfills.

Import & Usage

import { TypeRegistry } from '@sinclair/typebox'

TypeRegistry.Set('Foo', ...)

Would not be tree-shaken, because a function is invoked (which by default is considered to have side-effects, even when the module import itself is marked as free of side effects through sideEffects: false).

Note: the consuming projects sideEffects could come into play here, but that's the responsibility of consumers and won't be influenced by the value set in this package.

Import & Pure Usage

import { Type } from '@sinclair/typebox'

const MyType = /*#__PURE__*/ Type.Object({})
// eof

Would be fully tree-shaken with sideEffects: false, because MyType is unused and the invocation of Type.Object is marked as pure. Without the sideEffects flag, the import would be kept (same explanation as for the first case), but the Type.Object invocation would still be dropped, as it's flagged as pure.

Let me know if anything is unclear!