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

Parcel 2 lets babel private class polyfill bloat everything so much that an unused class adds *400 BYTES* WITH A PURE COMMENT #6116

Closed
danieltroger opened this issue Apr 11, 2021 · 29 comments

Comments

@danieltroger
Copy link
Contributor

🐛 bug report

Sometimes modules have sideeffects and usually terser eats them. If it doesn't, I can add a pure comment and then it eats them.

However the babel private class polyfill is so huge and weird (it's maybe less weird in loose mode but loose mode nearly produces equally huge outputs) that terser no longer removes all side effects.

Well, terser removes all side effects of my original code, but the babel transform causes side effects which terser doesn't remove.

I just noticed it's even worse: the babel artifacts are still in the output if there's just a class with private properties in a module, even if it's not even referenced from anywhere - try uncommenting everything before function used_export in module.ts

🎛 Configuration (.babelrc, package.json, cli command)

Please see attached .zip file.

🤔 Expected Behavior

!async function(){console.log("hi")}();

😯 Current Behavior

!function(){var e,t,n={},r={};t=r=function(e,t){return t.get?t.get.call(e):t.value},r.default=t,r.__esModule=true;var u,a=r,o={};u=o=function(e,t,n){if(!t.has(e))throw new TypeError("attempted to "+n+" private field on non-instance");return t.get(e)},o.default=u,o.__esModule=true;var l=o;e=n=function(e,t){var n=l(e,t,"get");return a(e,n)},n.default=e,n.__esModule=true;new WeakMap,new WeakSet;!async function(){console.log("hi")}()}();

(beautified for your convenience:

! function() {
  var e, t, n = {},
    r = {};
  t = r = function(e, t) {
    return t.get ? t.get.call(e) : t.value
  }, r.default = t, r.__esModule = true;
  var u, a = r,
    o = {};
  u = o = function(e, t, n) {
    if (!t.has(e)) throw new TypeError("attempted to " + n + " private field on non-instance");
    return t.get(e)
  }, o.default = u, o.__esModule = true;
  var l = o;
  e = n = function(e, t) {
    var n = l(e, t, "get");
    return a(e, n)
  }, n.default = e, n.__esModule = true;
  new WeakMap, new WeakSet;
  !async function() {
    console.log("hi")
  }()
}();

)

💁 Possible Solution

Add a pure comment before some function call of babel's output so that terser nukes it all.

🔦 Context

I want smaller output files.

💻 Code Sample

parcel-babel-bloat.zip

🌍 Your Environment

Software Version(s)
Parcel 2.0.0-nightly.635+f5962737
Node v15.13.0
npm/Yarn 7.7.6
Operating System macOS 10.15.7 (19H524)
@devongovett
Copy link
Member

Is this really a Babel issue?

@kzc
Copy link

kzc commented Apr 12, 2021

Workaround using a pure annotated IIFE:

var mysauce = /*@__PURE__*/(() => class mysauce {
  #privatevar = "sauce";
  #privatesauce() {
    return this.#privatevar;
  }
  givesauce() {
    return this.#privatesauce();
  }
})();
//console.log(new mysauce());

babel repl

I'm surprised that Babel doesn't add a pure wrapper for classes with private members automatically.

@mischnic
Copy link
Member

mischnic commented Apr 18, 2021

There isn't really anything we can do here. The problem is that Babel produces this suboptimal output (from the perspective of minifiers).

@danieltroger Do you want to open an issue with Babel describing how unused classes cannot be optimized away by terser if they contain private properties? (And include the example by kzc for a possible solution that could be implemented in the Babel transform)

@danieltroger
Copy link
Contributor Author

@kzc your workaround doesn't work.

Both changing

class mysauce extends Array {
  #privatevar = "sauce";

  #privatesauce() {
    return this.#privatevar;
  }

  givesauce() {
    return this.#privatesauce();
  }

}

to

var mysauce = /*@__PURE__*/(() => class mysauce extends Array {
  #privatevar = "sauce";
  #privatesauce() {
    return this.#privatevar;
  }
  givesauce() {
    return this.#privatesauce();
  }
})();

and just pasting your snippet into main.ts still has the babel artifacts.

Here's i.e. the output of just pasting your code into main.ts:

! function() {
  var e, t, r = {},
    n = {};
  t = n = function(e, t) {
    return t.get ? t.get.call(e) : t.value
  }, n.default = t, n.__esModule = true;
  var u, a = n,
    o = {};
  u = o = function(e, t, r) {
    if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance");
    return t.get(e)
  }, o.default = u, o.__esModule = true;
  var l = o;
  e = r = function(e, t) {
    var r = l(e, t, "get");
    return a(e, r)
  }, r.default = e, r.__esModule = true
}();
//# sourceMappingURL=modern.js.map

@danieltroger
Copy link
Contributor Author

@danieltroger Do you want to open an issue with Babel

Can I just link to this issue? Otherwise no, not really. Sorry for being ignorant but I got too much to do. Maybe in 11 days if I remember.

@kzc
Copy link

kzc commented Apr 19, 2021

The pure IIFE wrapper removed the only remnant of the class with private members seen in the top post:

new WeakMap, new WeakSet;

which was not present your subsequent post.

Parcel or some other process appears to be putting each of the Babel class helper functions into their own respective modules as evidenced by the three occurrences of .__esModule = true. Those functions can't be dropped due to the tangle of top level variable assignments and property settings which were not present in the babel repl link in my previous post. Perhaps bundling with the --no-minify CLI flag would offer more clues.

@mischnic
Copy link
Member

AFAICT, wrapping the class in a pure IIFE should work

Pasting the Babel output into https://try.terser.org/ removes everything

(However, this wrapping wouldn't be allowed in the general case once class static blocks are supported)

Can I just link to this issue?

You'd have to ask the Babel that 😄
I might open an issue soon then

@kzc
Copy link

kzc commented Apr 19, 2021

AFAICT, wrapping the class in a pure IIFE should work

I agree that all code should be dropped if ES6 is targeted, but the top post shows that ES5 style module code is being generated that prevents DCE:

n.default = e, n.__esModule = true;

Something in a .babelrc or Parcel scope hoisting perhaps?

@kzc
Copy link

kzc commented Apr 21, 2021

It appears that the babel runtime polyfills such as https://unpkg.com/@babel/runtime@7.13.17/helpers/classExtractFieldDescriptor.js:

function _classExtractFieldDescriptor(receiver, privateMap, action) {
  if (!privateMap.has(receiver)) {
    throw new TypeError("attempted to " + action + " private field on non-instance");
  }

  return privateMap.get(receiver);
}

module.exports = _classExtractFieldDescriptor;
module.exports["default"] = module.exports, module.exports.__esModule = true;

remain in ES5-style commonjs form after parcel bundling:

  var u, a = n, o = {};
  u = o = function(e, t, r) {
    if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance");
    return t.get(e)
  }, o.default = u, o.__esModule = true;

@parcel/babel-plugin-transform-runtime and @babel/plugin-transform-runtime don't appear to convert the babel runtime polyfills to ES6 form as documented.

The commonjs .default and .__esModule properties of each polyfill in the bundle prevent their dead code elimination despite being unused.

@kzc
Copy link

kzc commented Apr 21, 2021

How do you get parcel to pick up the esm versions of the babel runtime polyfills?

https://unpkg.com/@babel/runtime@7.13.17/helpers/esm/classExtractFieldDescriptor.js

export default function _classExtractFieldDescriptor(receiver, privateMap, action) {
  if (!privateMap.has(receiver)) {
    throw new TypeError("attempted to " + action + " private field on non-instance");
  }

  return privateMap.get(receiver);
}

@kzc
Copy link

kzc commented Apr 21, 2021

Okay, I get it... to force the use of esm babel runtime helpers you have to remove "@parcel/babel-plugin-transform-runtime" from .babelrc.

.babelrc:

{
  "presets": [
    "@babel/preset-typescript",
    "@parcel/babel-preset-env"
  ],
  "plugins": [
    "@babel/plugin-proposal-class-properties",
    "@babel/plugin-proposal-private-methods"
  ]
}

Then with this pure annotated version of src/module.ts:

export const mysauce = /*@__PURE__*/(() => class mysauce extends Array {
  constructor() {
    super();
    console.log(this.givesauce());
  }
  #privatevar = "sauce";
  #privatesauce() {
    return this.#privatevar;
  }
  givesauce() {
    return this.#privatesauce();
  }
})();

export async function used_export(){
  console.log("hi");
}

// uncomment to test class
//new mysauce;
//new mysauce;

parcel will produce:

$ node_modules/.bin/parcel build src/main.ts --log-level verbose --detailed-report 100 --target modern --no-cache
✨ Built in 1.10s

dist/modern.js                       75 B    150ms
├── src/module.ts                    39 B    102ms
└── Code from unknown sourcefiles    36 B      0ms

Notice that no artifacts of the class with private methods nor the babel runtime helpers are present in the final bundle:

$ cat dist/modern.js
!async function(){console.log("hi")}();
//# sourceMappingURL=modern.js.map

Edit: the pure IIFE wrapper example was updated to demonstrate that the unused exported class was dropped.

@mischnic
Copy link
Member

How do you get parcel to pick up the esm versions of the babel runtime polyfills?

This exports map is supposed to do that:

https://unpkg.com/browse/@babel/runtime@7.13.17/package.json

But Parcel doesn't apply it when resolving yet: #4155

@kzc
Copy link

kzc commented Apr 21, 2021

Deleting .babelrc altogether also works. Parcel will use esm default settings for Babel runtime helpers. It's only an issue with a user supplied custom .babelrc.

@kzc
Copy link

kzc commented Apr 21, 2021

afaict @parcel/babel-plugin-transform-runtime is now obsolete and increases bundle size in the worst case?

@mischnic
Copy link
Member

I'm not sure myself, the only case I know where you need that plugin is when transpiling away await/async (without it, you get regeneratorRuntime is not defined )

@danieltroger
Copy link
Contributor Author

@kzc thanks a lot for figuring this out! I think @mischnic is right, I'm using @parcel/babel-plugin-transform-runtime for async fns for my IE11 builds.

Without it the output is !function(){new WeakMap,new WeakSet;!async function(){console.log("hi")}()}();

And with the pure wrapped thing the output is correct with !async function(){console.log("hi")}();
I'll try opening a quick issue and ask babel to pure wrap it.

.babelrc is now another thing my build script needs to edit before calling parcel for different targets.
So that parcel builds for multiple targets in parallel is unfortunately not usable because:

  1. Different terser configs would be needed when targeting different language versions
  2. Evidently different babel configs also are needed when targeting different language versions
  3. Polyfills only need to be included in builds for older browsers

Imo per-target everything would be a much needed feature like I said in #5582
@mischnic can't you ask adobe for another dev or two to accelerate development and not drown in issues?

@mischnic
Copy link
Member

What we are trying to avoid is ending up where Webpack is, with a giant parcel.config.json containing these per-tool and per-target options.

Ideally, these points would be solved by improving Babel, that would then also benefit other projects/other build setups not using Parcel.

mischnic can't you ask adobe for another dev or two to accelerate development and not drown in issues?

I don't work for Adobe (anymore) myself, so there's nothing I can do in that regard.

@devongovett
Copy link
Member

The point of @parcel/babel-plugin-transform-runtime is to make babel aware of Parcel's targets. When building for an ESM environment, we use the ESM helpers. When building for CJS, we use the CJS helpers.

can't you ask adobe for another dev or two to accelerate development and not drown in issues?

Parcel is not an Adobe-run project, it's just where I happen to work in my day job (on completely different stuff). Adobe does not sponsor Parcel in any way.

@kzc
Copy link

kzc commented Apr 22, 2021

When building for an ESM environment, we use the ESM helpers. When building for CJS, we use the CJS helpers.

With the caveat above: #6116 (comment)

@danieltroger
Copy link
Contributor Author

What we are trying to avoid is ending up where Webpack is, with a giant parcel.config.json containing these per-tool and per-target options.

That makes sense. So what parcel really should do is automatically configuring terser and babel for the specified target

But what if someone does need to change an option?

I don't work for Adobe (anymore) myself, so there's nothing I can do in that regard.

Noo, we are doomed 😔

The point of @parcel/babel-plugin-transform-runtime is to make babel aware of Parcel's targets. When building for an ESM environment, we use the ESM helpers. When building for CJS, we use the CJS helpers.

The parcel output doesn't have any modules left anyways when using scope hoisting, so how can there be a difference between CSJ and ESM targets?

I guess there is one if one wants multiple output files, but that only happens when using dynamic imports and those use parcels own importing code.

Oooooooh, is it when the entry point is an html file?

I'm using parcel build src/main.ts like in the example zip, am I using CJS or ESM target then?

Parcel is not an Adobe-run project, it's just where I happen to work in my day job (on completely different stuff). Adobe does not sponsor Parcel in any way.

I thought I read some job posting somewhere and a discussion of how you could get paid for working on parcel. But it seems like it was Atlassian? I work at a small start up otherwise I'd ask my boss if he'd want to sponsor it lol.

But like how does it work? Parcel is great but has a bunch of issues and someone needs to pay to fix them. Or is the goal not to provide a great product but more to code on something for fun that people can use if they want?

When building for an ESM environment, we use the ESM helpers. When building for CJS, we use the CJS helpers.

With the caveat above: #6116 (comment)

So parcel does use the right helpers but because I explicitly said I want them it loaded them in a way where it isn't implemented yet that it loads them correctly?

@nicolo-ribaudo
Copy link

Parcel is great but has a bunch of issues and someone needs to pay to fix them.

That's true! The general expectation for free open source projects is that the "someone who pays for maintainance" are the users (individuals, or even better companies since they can pay more) of the project. Parcel accepts donations at https://opencollective.com/parcel!

@nicolo-ribaudo
Copy link

Cross-linking it here: I opened a PR in the Babel repo to add the pure annotations (babel/babel#13194)

@danieltroger
Copy link
Contributor Author

Parcel accepts donations at https://opencollective.com/parcel!

Yeah I saw that. Google gives a lot of money to webpack and I don't understand why they don't give it to parcel instead. Can't my taxes fund parcel? Imo OSS is public infrastructure

individuals

Ikr but I haven't come to terms yet that I should pay for the software I'm using at work. That's like paying to work.
Also I'm hesitant, what if we switch to esbuild because it's faster or something? Then my investments into parcel would be for nothing. Now that the argument that parcel is backed by adobe doesn't count anymore there's one reason less…

tl;dr: me: cheap & likes to complain

But thanks for all of your tireless work guys! I can't imagine having to use tsc and webpack instead. (Forreal I have a phobia)

@kzc
Copy link

kzc commented Apr 24, 2021

Thanks to the efforts of @alexlamsl, a yet to be released version of uglify-js has improved alias analysis and can now DCE the unused private class commonjs helpers, WeakMap, and WeakSet without modification to the original source code or babel:

$ cat parcel_bundle_6116.js | uglify-js -b
!function() {
    var e, t, n = {}, r = {};
    t = r = function(e, t) {
        return t.get ? t.get.call(e) : t.value;
    }, r.default = t, r.__esModule = true;
    var u, a = r, o = {};
    u = o = function(e, t, n) {
        if (!t.has(e)) throw new TypeError("attempted to " + n + " private field on non-instance");
        return t.get(e);
    }, o.default = u, o.__esModule = true;
    var l = o;
    e = n = function(e, t) {
        var n = l(e, t, "get");
        return a(e, n);
    }, n.default = e, n.__esModule = true;
    new WeakMap(), new WeakSet();
    !async function() {
        console.log("hi");
    }();
}();

Some non-default uglify-js minify options are necessary to achieve a smaller bundle size:

$ cat parcel_bundle_6116.js | uglify-js -mc passes=2
new WeakMap,new WeakSet,async function(){console.log("hi")}();

The uglify-js unsafecompress option is needed to remove unused built-in JS classes:

$ cat parcel_bundle_6116.js | uglify-js -mc passes=2,unsafe
!async function(){console.log("hi")}();

@mischnic
Copy link
Member

mischnic commented Apr 29, 2021

Should be fixed in in Babel 7.14.0

@kzc
Copy link

kzc commented Apr 29, 2021

The uglify-js optimization of the parcel generated babel helper commonjs code mentioned above has been released in uglify-js@3.13.5.

That release also adds ES2019 ESTree support and an extension for CallExpression.pure and NewExpression.pure which might be useful to the parcel project.

@mischnic
Copy link
Member

Nice, the last time I tried to feed the (converted) Babel AST into Terser, I had to give up because pure comments weren't being picked up.

However, we will soon switch to a string-based approach for generating the bundles, so there is no AST anyway.

BTW, would you say that uglify-js is better than Terser? Are there any (up-to-date) comparisons regarding performance/minification?

@kzc
Copy link

kzc commented Apr 29, 2021

@mischnic This is a recent benchmark: https://github.com/privatenumber/minification-benchmarks - but it's rather limited to libraries rather than entire bundled apps. https://github.com/mischnic/tree-shaking-example would be a better overall benchmark if updated with uglify-js support for the various bundlers (esbuild excluded).

Terser is a fork of uglify-es, which in turn was a fork of uglify-js. The recent ES2019 support in uglify-js was an alternate implementation done without the uglify-es or terser forks - even its internal uglify AST is different.

Better is a relative term. uglify-js has a number of more advanced optimizations, but that comes with the cost of using around 2X CPU for analysis. It all depends on the code base - sometimes uglify-js can produce bundles that are a few percent smaller, other times it's a wash. In the case of the commonjs helper code above, uglify-js can eliminate it completely, and terser cannot optimize it at all.

esbuild is popularizing the notion of a "good enough" level of optimization lately with strong emphasis on speed and simplicity over final bundle size.

I'm just bringing these uglify-js features to your attention because you mentioned that parcel AST use case a few years back. No worries if you choose not to use it.

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

5 participants