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

Add sideEffects: false flag to package.json to allow tree shaking #16059

Closed
3 of 12 tasks
bschlenk opened this issue Mar 26, 2019 · 14 comments
Closed
3 of 12 tasks

Add sideEffects: false flag to package.json to allow tree shaking #16059

bschlenk opened this issue Mar 26, 2019 · 14 comments

Comments

@bschlenk
Copy link

bschlenk commented Mar 26, 2019

Description of the problem

Consider adding "sideEffects": false to package.json so that bundlers like webpack are able to tree shake three.js. See https://webpack.js.org/guides/tree-shaking/#mark-the-file-as-side-effect-free for more info.

Three.js version
  • Dev
  • r102
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@andrevenancio
Copy link
Contributor

Hey! I just came to Github to propose something similar.
I'm just testing another javascript bundler (parcel.js) But I normally use webpack and rollup depending on the type of work I'm doing.

As an example I've created a simple end point which just has the following:

import { Vector3 } from 'three';
console.log(new Vector3(1, 2, 3));

I've compiled it, and I've created a 568.3kb file out of that. Mainly because all of threejs library came along with it.

Would adding the "sideEffects": false solve this issue entirely @bschlenk or do we need to refactor the exports across the threejs src in order to allow this to work?

It would greatly improve bundle sizes and website speeds if we could actually just use what we're importing from three.

Not sure if this can open up to a larger conversation on what's the core of threejs (cause that can be a long conversation on itself) but would be nice to have this feature added in!

@andrevenancio
Copy link
Contributor

andrevenancio commented Apr 4, 2019

I've quickly created a repo that has the above example running on all 3 bundlers I've mention (Parceljs, Rollupjs and Webpackjs)

https://github.com/andrevenancio/test-treeshaking

None of them seem to have process any tree shaking on three.js. Which validates @bschlenk point above. I did try however to copy the latest three.module.js to my src folder and import that instead (to avoid having to add "sideEffects": true I would expect that three shaking would happen in one of the bundlers, but they all fail to tree shake three.js.

import { Vector3 } from 'three';
console.log(new Vector3(1, 2, 3));

generates bundle sizes of:

parcel.js - 578kb
rollup.js - 553kb
webpack.js - 555kb

import { Vector3 } from 'three/src/math/vector3';
console.log(new Vector3(1, 2, 3));

generates bundle sizes of:

parcel.js - 16kb
rollup.js -14kb
webpack.js - 15kb

Now I'm confuse if this is a bundler problem and we should create an issue on their repos, or if this is a three.js specific issue.

I remember three.js has been heavily refactored by @Rich-Harris (author of Rollup) I would expect that tree shaking would at least work on his library. Surely I'm doing something wrong?

Maybe we should point to three.js source folder instead of the three.module.js file.

@bschlenk
Copy link
Author

bschlenk commented Apr 4, 2019

I pulled down your test package and did some experimenting. I was able to get tree shaking to work (I only tested webpack) after making two changes to three's package.json:

Change module to src/Three.js, and add "sideEffects": false.

I thought "sideEffects": false was the only change that needed to be made, but I guess not. I think that tree shaking in webpack is done at the module level. When using src/Three.js as the module, webpack can tree shake because this file has every single feature separated into a separate file, and Three.js just re-exports all of them.

The original file, build/three.module.js, has everything rolled into one file, so Webpack can't do tree shaking.

The sideEffects property also accepts an array of files which have side effects, I believe if this change were to actually be done, then src/polyfills.js would need to be listed there so that it doesn't get pruned out of the bundle.

@andrevenancio
Copy link
Contributor

Its possible that three.modules.js is for browser modules. But still your initial point is valid. package.json needs to be updated. I remember @TrySound changed gl-matrix quite a bit to make it possible to import it in any way possible. But I can't remember what he done exactly.

@vxna
Copy link

vxna commented Apr 4, 2019

Adding sideEffects: false alone will have no effect. With aliasing three to src/Three.js you'll lost both glsl and gl-constants build optimization and JSM imports support that fixes this long standing issue.

I wrote a plugin that helps situation for some, https://github.com/vxna/optimize-three-webpack-plugin

But I am all hands up for this change as it costs nothing and will be already there if something changes.

@bschlenk make a PR?

@EliasHasle
Copy link
Contributor

No PR yet?

Anyway, I think this issue thread could serve as a place to discuss the possibility, and obstacles for, tree-shaking three.js. Maybe it should be renamed.

@mrdoob
Copy link
Owner

mrdoob commented Sep 3, 2019

Can someone test if after #17276 this code produces small builds?

import { BoxBufferGeometry } from 'three';
console.log( new BoxBufferGeometry( 1, 2, 3 ) );

/ping @andrevenancio

@andrevenancio
Copy link
Contributor

Now sure I'm testing it the right way but I don't think it produces small builds.

I've pulled the PR from @Mugen87 on #17276 and did an npm run build on it.
I then pulled the dist/three.module to my repo and tried to build using parcel, rollup and webpack.

The only way I can get it to only import the BoxBufferGeometry instead of the whole library (resulting in a bundle of around 16kb instead of >500kb) is if I target the dependency directly like import { BoxBufferGeometry } from 'three/geometries/BoxGeometry'.

Though I think moving into class based components is a good decision moving forward, I'm not sure if its the only thing we need to do in order to get tree shaking working properly.

Would love to get some feedback from @Rich-Harris on what's the best approach for tree shake (in in rollup) and maybe a few of us can then start going through classes and adjusting where possible so we can aim towards smaller builds. I'm happy to help we just need a nudge in the right direction.

@Qubica
Copy link

Qubica commented Sep 26, 2019

Hi guys,

Just a few minor things need to happen if we want to treeshake threejs in webpack.

The first thing on this list is the only required change in the threejs package.

  1. add sideEffects parameter to the package.json
"sideEffects": [
  "src/Three.js"
]
  1. Set Webpack to production mode
mode: 'production'
  1. Set an alias for three inside the Webpack config:
resolve: {
  alias: {
    "three": "three/src/Three.js"
   }
}
  1. Include the TenserPlugin.
const TerserPlugin = require('terser-webpack-plugin');
optimization: {
    minimizer: [
        new TerserPlugin({
            extractComments: true,
            cache: false,
            parallel: true,
            sourceMap: false, // Must be set to true if using source-maps in production
            terserOptions: {
                extractComments: 'all',
                compress: {
                    pure_funcs: ['console.info', 'console.debug', 'console.log'] // remove info debug and log, keep warn and error
                },
                output: {
                    comments: false,
                },
            }
        })
    ]
}
  1. (Bonus) The BundleAnalyzerPlugin will help you see what it's the final bundle.
const BundleAnalyzerPlugin = require('webpack-bundle-analyzer').BundleAnalyzerPlugin;
plugins: [
    new BundleAnalyzerPlugin()
]

@vxna
Copy link

vxna commented Sep 26, 2019

@Qubica

Adding sideEffects: false alone will have no effect. With aliasing three to src/Three.js you'll lost both glsl and gl-constants build optimization and JSM imports support that fixes this long standing issue.

Also re-adding terser does nothing because it's already enabled in production mode.

@Rich-Harris
Copy link
Contributor

Would love to get some feedback from @Rich-Harris on what's the best approach for tree shake

Refactor the codebase to use classes. Everything else is displacement activity! (There may be additional refactoring needed after that to get everything to treeshake nicely, but that's the essential first step — it's hard to know what that work is until then)

@Qubica
Copy link

Qubica commented Sep 26, 2019

Adding sideEffects: false alone will have no effect. With aliasing three to src/Three.js you'll lost both glsl and gl-constants build optimization and JSM imports support that fixes this long standing issue.

Hmm, I had to read better. You're right, my bad.

@Mugen87
Copy link
Collaborator

Mugen87 commented Sep 27, 2019

Closing for now. As mentioned by @Rich-Harris, we focus now on the migration to ES6 classes (e.g. #17425). After that, we'll evaluate what's to do next in order to get treeshaking to work.

@yushijinhun
Copy link
Contributor

I've just developed a webpack/rollup plugin three-minifier to solve this problem.

peterpeterparker added a commit to dfinity/verifiable-credentials-sdk that referenced this issue Sep 10, 2024
# Motivation

It was reported that the library does not work with Next.JS but, we were
not abole to reproduce the issue. However, the developer is using a
custom Webpack, that's why we can actually assume that the issue related
to this bundle.

On another side, the issue does not seems to reside solely in the build
but, also when imported in an editor as Webstorm or VS Code does not
seem able to understand the lib is modular.

That's why, there are various things we should do to fix the problem.

# Webpack

Webpack seems to require a `sideEffects` flag in `package.json` to
enable tree shaking for a library
([documentation](https://webpack.js.org/guides/tree-shaking/)). This
was, for example, introduced in
[three.js](mrdoob/three.js#16059).

I'm also unsure whether Webpack requires a `main` field. This PR adds
the main entry, even though it will still resolve to an index that
throws an error, since we are providing a modular library.

# Import and validation

When using `exports` in `package.json` it seems that editor have troube
understanding that the modules are not shipped at the root of the
library. I did not find a solution therefore decided to adapt the way we
provide the information by moving `package.json`, and other metadata
files` to the `dist` folder. That way the package can reference files at
the same level.

⚠️ This comes with an important change. The library after `npm run
build` should not be published anymore from the root of the library but,
from the `dist` folder. That is why the pipelines had to be updated.

# Other changes

While fixing those issues I noticed few other minor problems. The
`mocks.ts` was packaged in the lib and the file `LICENSE` was missing.
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

9 participants