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

Remove 'sideEffects' field from package.jsons #7057

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mattsoulanille
Copy link
Member

@mattsoulanille mattsoulanille commented Nov 17, 2022

Some of tfjs's package.json files have a sideEffects field to mark certain files as side-effectful for bundlers. However, bundlers can do static analysis to find side effects automatically, and this is usually more accurate and less error-prone than relying on a manually maintained list.

This PR removes the sideEffects field from the package.json files.

Fixes #4259
Fixes #5182

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@mattsoulanille
Copy link
Member Author

I've run nightly tests on this PR and found no failing tests. I still need to check if this change affects bundle sizes.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained

@mattsoulanille
Copy link
Member Author

I'm seeing some size differences in custom bundles:
Without this change:

1000K	dist/full/app_rollup.js
432K	dist/custom/app_rollup.js

With this change:

1004K	dist/full/app_rollup.js
524K	dist/custom/app_rollup.js

Without sideEffects, rollup seems to be ignoring the custom bundle's list of ops and including everything.

@mattsoulanille
Copy link
Member Author

This may require a more extensive refactor than I initially anticipated. tfjs-core exports all the ops from ops.ts (these are eventually exported in index.ts), which I think means they can not be tree-shaken. Custom bundles replace the ops_for_converter.ts file (which re-exports all the ops) with a new file that exports only a subset of the ops. However, they still export all of tfjs-core's base.ts, which re-exports all the ops, so I'm not really sure how this works when sideEffects is set in the package.json but not when it's not set.

`train.ts` exports optimizers by copying them from the `OptimizerConstructors` class onto a `train` object. This is unnecessary because the `OptimizerConstructors` class constructor is a subtype of the `train` object's type (i.e. it has all the properties that `train` has). Instead of creating a new `train` object, this PR re-exports `OptimizerConstructors` as `train`.

This has no direct effect now, but if / when re remove the `sideEffects` field from `package.json`, it helps some bundlers (esbuild) do tree-shaking.
Each `Optimizer` lists its class name as a static property of the class so it can be serialized and deserialized. This prevents the class from being tree-shaken because bundlers will compile it like this:

```
class SomeOptimizer {
  ...
}

// The bundler can not remove this assignment because
// SomeOptimizer.className could be a setter with a side effect.
SomeOptimizer.className = 'SomeOptimizer';
```

This PR uses a static getter for the class name instead, which bundlers can tree-shake properly.
Enable the TypeScript [`esModuleInterop` flag](https://www.typescriptlang.org/tsconfig#esModuleInterop), which was released in TS2.7 and is a recommended config option. This removes the need for some workarounds we use for rollup plugins and Long.

Upgrade Long from 4.0.0 to 5.2.1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants