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

fix: typebox imports not being tree shaken #909

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

marcesengel
Copy link

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.

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`.
@sinclairzx81
Copy link
Owner

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

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

Successfully merging this pull request may close these issues.

None yet

2 participants