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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tree shaking library builds doesn't work #8676

Open
danieltroger opened this issue Dec 6, 2022 · 9 comments
Open

Tree shaking library builds doesn't work #8676

danieltroger opened this issue Dec 6, 2022 · 9 comments

Comments

@danieltroger
Copy link
Contributor

danieltroger commented Dec 6, 2022

馃悰 bug report

When building a library using parcel and then building something that consumes that library, using parcel, tree shaking is not working how it should

馃帥 Configuration (.babelrc, package.json, cli command)

A yarn workspace with an inner library package that's built as library and an outer package that's built for the browser, see https://github.com/danieltroger/parcel-tree-shaking-bug

Run yarn rebuild-everything to rebuild the library and code consuming it.

馃 Expected Behavior

Given that we're just doing

import {} from "issue-reproduction-library";

and our library is pure - it only contains functions, except for this function call which has a pure comment - the browser build (this file) should be an empty file.

馃槸 Current Behavior

The browser build (this file) contains the whole library code.

馃拋 Possible Solution

Two things need to be done to achieve the desired 0 byte browser output in the example repo:

  1. $parcel$export needs to be prefixed with a pure comment
  2. The pure comment in /*@__PURE__*/ (0, $b208948437c85312$export$f5e092cdfed98805) needs to be preserved as part of the function name by parcel when it does the (0, function_name) thing, which I understand it needs to do so the function has the correct value for this. So it should be (0, /*@__PURE__*/ $b208948437c85312$export$f5e092cdfed98805) instead.

For further clarity, see this diff, which is how I would propose parcel to modify it's output for the library build: danieltroger/parcel-tree-shaking-bug@fbca029

Screenshot 2022-12-06 at 15 19 33

馃敠 Context

We're starting to use Yarn's workspace features. We're splitting up big parts of our repo into different packages since we want to publish packages on npm and they need to be able to import our, right now private, code which they depend on from a public package. After days of agonising updating of import statements and type error fixing we have given up on splitting up our codebase into multiple packages. This is because we got a lot of circular dependencies by the imports, which broke yarn serve (it would be awesome if parcel would warn for this and show where they are!). Instead we will move everything into one huge package.

Here it is paramount though that tree shaking works. Right now, even importing nothing from the utility package we have created adds 125.53KB to whatever is using it. I have debugged it and it seems like it boils down to the two issues mentioned here. I would very much appreciate if this could be fixed in the near future. If my proposed solution works I imagine I could even do it myself given the locations in the parcel code where the pure comments should be added.

馃捇 Code Sample

https://github.com/danieltroger/parcel-tree-shaking-bug

馃實 Your Environment

Software Version(s)
Parcel 2.0.0-nightly.1209+ae31c3c85 (in reproduction repo), actually using 2.8.0
Node v19.2.0
npm/Yarn 3.2.2
Operating System macOS 13.0.1 (22A400)
@devongovett
Copy link
Member

devongovett commented Dec 6, 2022

I guess the question is why an exports namespace object is included here at all in the first place...

@michael42
Copy link

Maybe you can work around that by using a slightly different @PURE comment that uses an IIFE?

export const library_function = /* @__PURE__*/ (() =>
    catch_errors(() => {
        console.log("I just did very complex stuff. Imagine there are very many functions like me!")
    })
)()

It's a bit ugly, but it worked in my case (except that the imported library does not have an ESM version, so I had to alias it away, additionally). That syntax has the additional benefit of flagging an entire code block as pure, avoiding having to write stuff like this:

import {Type} from "@sinclair/typebox"

export const SomeModel = /* @__PURE__*/ Type.Object({
    fieldA: /* @__PURE__*/ Type.String(),
    fieldB: /* @__PURE__*/ Type.String(),
    fieldC: /* @__PURE__*/ Type.Number(),
})

@danieltroger
Copy link
Contributor Author

danieltroger commented Jan 19, 2023

Maybe you can work around that by using a terser/terser#513 (comment) that uses an IIFE?

That works!

But I spent like one hour on changing export * to exporting everything and doing export type for the types and 45 minutes on doing extra IIFE wrapping and it sucks. We have so many places that get messed up by the pure comment being in the wrong place when parcel puts (0, before the function call that I'm giving up on it now.

IMO the damage of breaking tree shaking is bigger than the damage caused by having the wrong this for some functions (our whole codebase doesn't care aboutthis in that case for example).

@danieltroger
Copy link
Contributor Author

The pure (0,-bug also very much affects users of solid-js. Solid-js gets included and all solid-js components that exist anywhere get included because solid's JSX transform has a copy of every element that it then clones in the module scope.

So something like the highlighted line here:

Screenshot 2023-01-19 at 18 01 20

Becomes something like const $f19e81a7eecb0aac$var$_tmpl$ = /*#__PURE__*/ (0, $g9l4l$template)(, 2), and then no longer gets removed

@danieltroger
Copy link
Contributor Author

I'm currently writing a babel transformer with chatGPT to fix the pure comment part of this because I had to write another one to rewrite some imports because most of our customers struggle with transpiling javascript. I just wanted to ask: what you're saying with

I guess the question is why an exports namespace object is included here at all in the first place...

is that all calls to $parcel$export can be completely dropped in the esmodule outputs? So I can just add pure comments in front of them so terser removes them completely?

@danieltroger
Copy link
Contributor Author

danieltroger commented Jul 21, 2023

Nevermind, I answered my question myself: it doesn't always suffice, sometimes there are also export statements but sometimes nothing is exported when dropping the $parcel$export calls

edit: nevermind on the nevermind, I think there are always also export statements - my files had the wrong contents (commonjs and esmodule mixed up)

@tonai
Copy link

tonai commented Jan 15, 2024

I also have the same issue.
Just trying to build a simple library with Parcel.
I have an entry point index.ts that re-export 2 things:

export * from './big'; // contains 1 big constant
export * from './helpers'; // contains 2 functions

In my library package.json I have:

{
  "type": "module",
  "source": "src/index.ts",
  "main": "dist/parcel-lib.cjs",
  "module": "dist/parcel-lib.mjs",
  "types": "dist/index.d.ts",
}

If I use my library in my app that does not make use of the constant (originally located in the big.ts file) and use Parcel to build the app, then the constant is not included in the final bundle.
But if I use webpack, vite or any other bundler for my app, then the tree-shaking is not working and the constant gets included in the final bundle.

@contactjavas
Copy link

Also getting same/similar issue. Named export functions that we don't import keep included in the build.
I think those functions don't even have side effects.

@contactjavas
Copy link

Oh, in my case, this is because I build 2 files in one command like this:

{
    "build-site-js": "parcel build assets/js/site.js assets/js/site-jquery.js --dist-dir js/min --no-source-maps"
}

So the site-jquery.js just import few functions and the site.js (basically, non jQuery version) imports more functions.
Using above command, the functions that I don't import in site-jquery.js keep included in the build file.

But splitting that command into below, fix my issue:

{
    "build-site-jquery": "parcel build assets/js/site-jquery.js --dist-dir js/min --no-source-maps",
    "build-site-js": "parcel build assets/js/site.js --dist-dir js/min --no-source-maps"
}

By splitting it, functions that I don't import in site-jquery.js are not included in the bundle.

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

6 participants