Addons: Add PURE annotations for tree-shaking. #26912
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
Just a comment - this may be the kind of thing that's best done in stages so it's easier for project members to review and approve. Ie a PR for just adding |
|
Making a PR that just adds the |
|
There seems to be three recurring patterns used in this PR:
I'm familiar with the first one since we are already using this approach in the core. Do you mind explain 2 and 3? It's important that all collaborators (including myself^^) fully understand these patterns to ensure maintenance. Especially the IIFE thing is something that could easily be overlooked in new code that is going to be added to the library. |
|
The use of // Assumes tree-shaking is at module level
class Foo {}
/* @__PURE__ */ Object.assign( Foo, { staticProperty: true } );// Assumes tree-shaking is at module level by value
const Foo = /* @__PURE__ */ ( () => {
class Foo {}
Foo.staticProperty = true
return Foo;
} )();
|
Related (in some way) issue: #25526 |
See latter case in #26912 (comment).
|
Looks like the latter case in #26912 (comment) is true. This adds a lot of whitespace nonsense, but otherwise the files are unmodified. Still, this would be good to split up since the rest of this PR very cleanly adds annotations. Edit: I've since removed the whitespace changes, but they will need to be formatted with a linter. Actual code changes should just be the scope for the IIFE. |
| const CstParser = chevrotain.CstParser; | ||
| const { CstParser } = chevrotain; |
There was a problem hiding this comment.
Perhaps a better example of #26912 (comment). I refactored this from a static method back to a class in b9d5666, figuring that destructuring from chevrotain rather than member access would tree-shake and that seems to be the case. Why they're not equivalent for tools, I'm not sure. Perhaps it's accounting for possible getters? This case is regarded in https://esbuild.github.io/api/#tree-shaking-and-side-effects.
There was a problem hiding this comment.
In any event, I like this solution more than the previous refactoring 😇 .
|
I've also recapped everything found in review in the PR description for future reference with some notes on future quality control via linting. For this PR and future reference, I've written a CLI tool to E2E test with bundlers and diagnostics which I describe further there. |
| return lottie; | ||
|
|
||
| } ); | ||
|
|
||
| export default lottie; |
There was a problem hiding this comment.
from my tests, this optimization and most of the optimizations inside examples/jsm/libs are not needed
There was a problem hiding this comment.
After further review I have to affirm that they are necessary and precisely for the issues I outlined in the PR description. Rather than individually resolve these like I did in #26935, I thought it better to wrap their scopes in an IIFE for the sake of review.
There was a problem hiding this comment.
Not sure how you're testing, but I've made a repro branch of what I mean:
https://github.com/marcofugaro/treeshake-test/tree/addons-test
As you can see LottieLoader and lottie_canvas.module.js are tree-shaken already.
I also tested fflate.module.js, opentype.module.js, mmdparser.module.js but I believe most of jsm/libs are already tree-shakeable.
There was a problem hiding this comment.
As a matter of fact even other addons are tree-shaken already, I think having the proxy Addons.js is enough to make all addons tree-shakeable.
I tested with this
// Addons.js
export * from './loaders/LottieLoader.js';
export * from './loaders/TTFLoader.js';
export * from './loaders/TiltLoader.js';
export * from './loaders/USDZLoader.js';
export * from './loaders/VOXLoader.js';
export * from './loaders/VRMLLoader.js';
export * from './loaders/VTKLoader.js';
export * from './loaders/XYZLoader.js';
export * from './loaders/lwo/IFFParser.js';
export * from './loaders/lwo/LWO2Parser.js';
export * from './loaders/lwo/LWO3Parser.js';// index.js
import { LottieLoader } from 'three/addons'
console.log(LottieLoader)And they all were already tree-shaken except LottieLoader (I am not testing this PR, but default three). But maybe I'm wrong, could you test this?
There was a problem hiding this comment.
I can provide repro of this if needed.
There was a problem hiding this comment.
As per my last message, I explained the aforementioned issues and testing strategy in the PR description. I'm not testing any optimizers like Uglify or Terser (Vite optionally uses the latter), nor am I treating three as external in the event that package.sideEffects is interactive. Please understand that I'm regarding the lowest common denominator between minification strategies for creating a baseline for testing and the sake of observability, so even though tools like ESBuild and Rollup have access to package information for smarter traversal, others only work at the module level and can't afford such optimizations.
There was a problem hiding this comment.
Yeah I did read the description. My point is that in the real world, those issues don't prevent the tree-shaking of #26910.
Unlike the main bundle, we're not importing from a single file here. Since the package.sideEffects flag is present, the bundlers are able to completely discard the unused files, even if they have some side-effects in it.
I did a test with the full Addons.js from #26910 and as you can see Esbuild, Rollup and Webpack can successfully tree-shake the Addons.js file, even without minification (only three.module.js and FXAAShader.js are present in the final bundle):
https://github.com/marcofugaro/treeshake-test/blob/addons-test-full/dist/index-esbuild.js
My point is, the changes in jsm/libs and the IIFE wraps make maintaning the repo more difficult, while not providing any real improvement to tree-shaking.
|
Closing since we can rely on |
Related issue: #24595
Description
Backports tree-shaking fixes for addons from
three-stdlib. This enablesthree/addonsin #26910.I haven't touched WebGPU or nodes code since they are WIP and won't be included in #26910, but I have a branch handy where I've already investigated tree-shaking prior which is uniquely effectful (related: #25526).
Progress:
three/addons/animationthree/addons/animation/AnimationClipCreator.jsthree/addons/animation/CCDIKSolver.jsthree/addons/animation/MMDAnimationHelper.jsthree/addons/animation/MMDPhysics.jsthree/addons/camerasthree/addons/cameras/CinematicCamera.jsthree/addons/capabilitiesthree/addons/capabilities/WebGL.jsthree/addons/controlsthree/addons/controls/ArcballControls.jsthree/addons/controls/DragControls.jsthree/addons/controls/FirstPersonControls.jsthree/addons/controls/FlyControls.jsthree/addons/controls/MapControls.jsthree/addons/controls/OrbitControls.jsthree/addons/controls/PointerLockControls.jsthree/addons/controls/TrackballControls.jsthree/addons/controls/TransformControls.jsthree/addons/csmthree/addons/csm/CSM.jsthree/addons/csm/CSMFrustum.jsthree/addons/csm/CSMHelper.jsthree/addons/csm/CSMShader.jsthree/addons/curvesthree/addons/curves/CurveExtras.jsthree/addons/curves/NURBSCurve.jsthree/addons/curves/NURBSSurface.jsthree/addons/curves/NURBSUtils.jsthree/addons/effectsthree/addons/effects/AnaglyphEffect.jsthree/addons/effects/AsciiEffect.jsthree/addons/effects/OutlineEffect.jsthree/addons/effects/ParallaxBarrierEffect.jsthree/addons/effects/PeppersGhostEffect.jsthree/addons/effects/StereoEffect.jsthree/addons/environmentsthree/addons/environments/DebugEnvironment.jsthree/addons/environments/RoomEnvironment.jsthree/addons/exportersthree/addons/exporters/DRACOExporter.jsthree/addons/exporters/EXRExporter.jsthree/addons/exporters/GLTFExporter.jsthree/addons/exporters/KTX2Exporter.jsthree/addons/exporters/MMDExporter.jsthree/addons/exporters/OBJExporter.jsthree/addons/exporters/PLYExporter.jsthree/addons/exporters/STLExporter.jsthree/addons/exporters/USDZExporter.jsthree/addons/geometriesthree/addons/geometries/BoxLineGeometry.jsthree/addons/geometries/ConvexGeometry.jsthree/addons/geometries/DecalGeometry.jsthree/addons/geometries/ParametricGeometries.jsthree/addons/geometries/ParametricGeometry.jsthree/addons/geometries/RoundedBoxGeometry.jsthree/addons/geometries/TeapotGeometry.jsthree/addons/geometries/TextGeometry.jsthree/addons/helpersthree/addons/helpers/LightProbeHelper.jsthree/addons/helpers/OctreeHelper.jsthree/addons/helpers/PositionalAudioHelper.jsthree/addons/helpers/RectAreaLightHelper.jsthree/addons/helpers/VertexNormalsHelper.jsthree/addons/helpers/VertexTangentsHelper.jsthree/addons/helpers/ViewHelper.jsthree/addons/interactivethree/addons/interactive/HTMLMesh.jsthree/addons/interactive/InteractiveGroup.jsthree/addons/interactive/SelectionBox.jsthree/addons/interactive/SelectionHelper.jsthree/addons/lightsthree/addons/lights/IESSpotLight.jsthree/addons/lights/LightProbeGenerator.jsthree/addons/lights/RectAreaLightUniformsLib.jsthree/addons/linesthree/addons/lines/Line2.jsthree/addons/lines/LineGeometry.jsthree/addons/lines/LineMaterial.jsthree/addons/lines/LineSegments2.jsthree/addons/lines/LineSegmentsGeometry.jsthree/addons/lines/Wireframe.jsthree/addons/lines/WireframeGeometry2.jsthree/addons/loadersthree/addons/loaders/3DMLoader.jsthree/addons/loaders/3MFLoader.jsthree/addons/loaders/AMFLoader.jsthree/addons/loaders/BVHLoader.jsthree/addons/loaders/ColladaLoader.jsthree/addons/loaders/DDSLoader.jsthree/addons/loaders/DRACOLoader.jsthree/addons/loaders/EXRLoader.jsthree/addons/loaders/FBXLoader.jsthree/addons/loaders/FontLoader.jsthree/addons/loaders/GCodeLoader.jsthree/addons/loaders/GLTFLoader.jsthree/addons/loaders/HDRCubeTextureLoader.jsthree/addons/loaders/IESLoader.jsthree/addons/loaders/KMZLoader.jsthree/addons/loaders/KTX2Loader.jsthree/addons/loaders/KTXLoader.jsthree/addons/loaders/LDrawLoader.jsthree/addons/loaders/LUT3dlLoader.jsthree/addons/loaders/LUTCubeLoader.jsthree/addons/loaders/LWOLoader.jsthree/addons/loaders/LogLuvLoader.jsthree/addons/loaders/LottieLoader.jsthree/addons/loaders/MD2Loader.jsthree/addons/loaders/MDDLoader.jsthree/addons/loaders/MMDLoader.jsthree/addons/loaders/MTLLoader.jsthree/addons/loaders/NRRDLoader.jsthree/addons/loaders/OBJLoader.jsthree/addons/loaders/PCDLoader.jsthree/addons/loaders/PDBLoader.jsthree/addons/loaders/PLYLoader.jsthree/addons/loaders/PVRLoader.jsthree/addons/loaders/RGBELoader.jsthree/addons/loaders/RGBMLoader.jsthree/addons/loaders/STLLoader.jsthree/addons/loaders/SVGLoader.jsthree/addons/loaders/TDSLoader.jsthree/addons/loaders/TGALoader.jsthree/addons/loaders/TIFFLoader.jsthree/addons/loaders/TTFLoader.jsthree/addons/loaders/TiltLoader.jsthree/addons/loaders/USDZLoader.jsthree/addons/loaders/VOXLoader.jsthree/addons/loaders/VRMLLoader.jsthree/addons/loaders/VTKLoader.jsthree/addons/loaders/XYZLoader.jsthree/addons/loaders/lwo/IFFParser.jsthree/addons/loaders/lwo/LWO2Parser.jsthree/addons/loaders/lwo/LWO3Parser.jsthree/addons/materialsthree/addons/materials/MeshGouraudMaterial.jsthree/addons/maththree/addons/math/Capsule.jsthree/addons/math/ColorConverter.jsthree/addons/math/ConvexHull.jsthree/addons/math/ImprovedNoise.jsthree/addons/math/Lut.jsthree/addons/math/MeshSurfaceSampler.jsthree/addons/math/OBB.jsthree/addons/math/Octree.jsthree/addons/math/SimplexNoise.jsthree/addons/miscthree/addons/misc/ConvexObjectBreaker.jsthree/addons/misc/GPUComputationRenderer.jsthree/addons/misc/Gyroscope.jsthree/addons/misc/MD2Character.jsthree/addons/misc/MD2CharacterComplex.jsthree/addons/misc/MorphAnimMesh.jsthree/addons/misc/MorphBlendMesh.jsthree/addons/misc/ProgressiveLightMap.jsthree/addons/misc/RollerCoaster.jsthree/addons/misc/TubePainter.jsthree/addons/misc/Volume.jsthree/addons/misc/VolumeSlice.jsthree/addons/modifiersthree/addons/modifiers/CurveModifier.jsthree/addons/modifiers/EdgeSplitModifier.jsthree/addons/modifiers/SimplifyModifier.jsthree/addons/modifiers/TessellateModifier.jsthree/addons/objectsthree/addons/objects/GroundProjectedSkybox.jsthree/addons/objects/Lensflare.jsthree/addons/objects/MarchingCubes.jsthree/addons/objects/Reflector.jsthree/addons/objects/ReflectorForSSRPass.jsthree/addons/objects/Refractor.jsthree/addons/objects/ShadowMesh.jsthree/addons/objects/Sky.jsthree/addons/objects/Water.jsthree/addons/objects/Water2.jsthree/addons/physicsthree/addons/physics/AmmoPhysics.jsthree/addons/physics/RapierPhysics.jsthree/addons/postprocessingthree/addons/postprocessing/AfterimagePass.jsthree/addons/postprocessing/BloomPass.jsthree/addons/postprocessing/BokehPass.jsthree/addons/postprocessing/ClearPass.jsthree/addons/postprocessing/CubeTexturePass.jsthree/addons/postprocessing/DotScreenPass.jsthree/addons/postprocessing/EffectComposer.jsthree/addons/postprocessing/FilmPass.jsthree/addons/postprocessing/GlitchPass.jsthree/addons/postprocessing/HalftonePass.jsthree/addons/postprocessing/LUTPass.jsthree/addons/postprocessing/MaskPass.jsthree/addons/postprocessing/OutlinePass.jsthree/addons/postprocessing/OutputPass.jsthree/addons/postprocessing/Pass.jsthree/addons/postprocessing/RenderPass.jsthree/addons/postprocessing/RenderPixelatedPass.jsthree/addons/postprocessing/SAOPass.jsthree/addons/postprocessing/SMAAPass.jsthree/addons/postprocessing/SSAARenderPass.jsthree/addons/postprocessing/SSAOPass.jsthree/addons/postprocessing/SSRPass.jsthree/addons/postprocessing/SavePass.jsthree/addons/postprocessing/ShaderPass.jsthree/addons/postprocessing/TAARenderPass.jsthree/addons/postprocessing/TexturePass.jsthree/addons/postprocessing/UnrealBloomPass.jsthree/addons/renderersthree/addons/renderers/CSS2DRenderer.jsthree/addons/renderers/CSS3DRenderer.jsthree/addons/renderers/Projector.jsthree/addons/renderers/SVGRenderer.jsthree/addons/shadersthree/addons/shaders/ACESFilmicToneMappingShader.jsthree/addons/shaders/AfterimageShader.jsthree/addons/shaders/BasicShader.jsthree/addons/shaders/BleachBypassShader.jsthree/addons/shaders/BlendShader.jsthree/addons/shaders/BokehShader.jsthree/addons/shaders/BokehShader2.jsthree/addons/shaders/BrightnessContrastShader.jsthree/addons/shaders/ColorCorrectionShader.jsthree/addons/shaders/ColorifyShader.jsthree/addons/shaders/ConvolutionShader.jsthree/addons/shaders/CopyShader.jsthree/addons/shaders/DOFMipMapShader.jsthree/addons/shaders/DepthLimitedBlurShader.jsthree/addons/shaders/DigitalGlitch.jsthree/addons/shaders/DotScreenShader.jsthree/addons/shaders/ExposureShader.jsthree/addons/shaders/FXAAShader.jsthree/addons/shaders/FilmShader.jsthree/addons/shaders/FocusShader.jsthree/addons/shaders/FreiChenShader.jsthree/addons/shaders/GammaCorrectionShader.jsthree/addons/shaders/GodRaysShader.jsthree/addons/shaders/HalftoneShader.jsthree/addons/shaders/HorizontalBlurShader.jsthree/addons/shaders/HorizontalTiltShiftShader.jsthree/addons/shaders/HueSaturationShader.jsthree/addons/shaders/KaleidoShader.jsthree/addons/shaders/LuminosityHighPassShader.jsthree/addons/shaders/LuminosityShader.jsthree/addons/shaders/MMDToonShader.jsthree/addons/shaders/MirrorShader.jsthree/addons/shaders/NormalMapShader.jsthree/addons/shaders/OutputShader.jsthree/addons/shaders/RGBShiftShader.jsthree/addons/shaders/SAOShader.jsthree/addons/shaders/SMAAShader.jsthree/addons/shaders/SSAOShader.jsthree/addons/shaders/SSRShader.jsthree/addons/shaders/SepiaShader.jsthree/addons/shaders/SobelOperatorShader.jsthree/addons/shaders/SubsurfaceScatteringShader.jsthree/addons/shaders/TechnicolorShader.jsthree/addons/shaders/ToonShader.jsthree/addons/shaders/TriangleBlurShader.jsthree/addons/shaders/UnpackDepthRGBAShader.jsthree/addons/shaders/VelocityShader.jsthree/addons/shaders/VerticalBlurShader.jsthree/addons/shaders/VerticalTiltShiftShader.jsthree/addons/shaders/VignetteShader.jsthree/addons/shaders/VolumeShader.jsthree/addons/shaders/WaterRefractionShader.jsthree/addons/texturesthree/addons/textures/FlakesTexture.jsthree/addons/utilsthree/addons/utils/BufferGeometryUtils.jsthree/addons/utils/CameraUtils.jsthree/addons/utils/GPUStatsPanel.jsthree/addons/utils/GeometryCompressionUtils.jsthree/addons/utils/GeometryUtils.jsthree/addons/utils/LDrawUtils.jsthree/addons/utils/PackedPhongMaterial.jsthree/addons/utils/SceneUtils.jsthree/addons/utils/ShadowMapViewer.jsthree/addons/utils/SkeletonUtils.jsthree/addons/utils/TextureUtils.jsthree/addons/utils/UVsDebug.jsthree/addons/utils/WorkerPool.jsthree/addons/webxrthree/addons/webxr/ARButton.jsthree/addons/webxr/OculusHandModel.jsthree/addons/webxr/OculusHandPointerModel.jsthree/addons/webxr/Text2D.jsthree/addons/webxr/VRButton.jsthree/addons/webxr/XRButton.jsthree/addons/webxr/XRControllerModelFactory.jsthree/addons/webxr/XREstimatedLight.jsthree/addons/webxr/XRHandMeshModel.jsthree/addons/webxr/XRHandModelFactory.jsthree/addons/webxr/XRHandPrimitiveModel.jsthree/addons/webxr/XRPlanes.jsIssues
The issues I've come across for tree-shaking are:
/* @__PURE__ */or/* #__PURE__ */Object.createandFileReaderas used here are properly impure, most others aren'tstaticfields are ES2022, methods ES6)Object.assignat the module level, but without a value attached to them, they get discardedTesting
For testing, I've written https://github.com/CodyJasonBennett/treeshake-cli which tests against the popular bundlers --
rollup,webpack, andesbuild(viteusesesbuildin dev androllupin prod). Issues such as mentioned above are reported.With
npx treeshake-cli@latest examples/jsm/Addons.jsfrom #26910:Linting
For linting, so you can get a gentle nudge in CI (as it's likely best effort in core/nodes), I've come across https://github.com/lukastaegert/eslint-plugin-tree-shaking which does not implement support for pure annotations yet. I plan to implement this and possibly use it with the above CLI tool, but will be invaluable here for non-intrusive regression testing.
For files with IIFEs used for static fields, I've collapsed whitespace changes for the sake of review (for why this matters, see #26935 (comment)). In IDE, there will be lint warnings, so they will have to be formatted once greenlighted. See e4fa50b for affected files.