emberjs / ember-cli-babel

Ember CLI plugin for Babel
MIT License
153 stars 119 forks source link

[Bug] browserslist and targets have no effect on the output of the build (potential performance problems as well) #417

Open NullVoxPopuli opened 2 years ago

NullVoxPopuli commented 2 years ago

Originally reported here: https://github.com/emberjs/ember.js/issues/19790

🐞 Describe the Bug

After some investigation here: https://stackoverflow.com/questions/69547969/with-babel-how-do-i-not-compile-away-class-properties-since-browsers-support And a minimal babel repro here: https://github.com/NullVoxPopuli/babel-transpilation-tests I expect that setting my targets to last 1 firefox versions would allow me to keep native class properties. and only transpile what is decororated.

I believe fixing this could provide a bit of a boost to everyone's build times. Today all class properties are transpiled to a time when class properties were not implemented in the browser).

πŸ”¬ Minimal Reproduction

πŸ˜• Actual Behavior

Living in the dark ages

πŸ€” Expected Behavior

Utilize support for native properties, private properties, methods, etc.

🌍 Environment

βž• Additional Context

See the comparison between these two files:

Input: https://github.com/NullVoxPopuli/babel-transpilation-tests/blob/main/input.js Output: https://github.com/NullVoxPopuli/babel-transpilation-tests/blob/main/output.js

This is correct. But when I add that input file to ember, I get this for the output:

/home/nullvoxpopuli/Development/tmp/my-app/my-app/input.js: Class private methods are not enabled.
  2 |   #a = 'hi';
  3 |
> 4 |   get #b() {
    |   ^
  5 |     return 'hello' + this.#a;
  6 |   }
  7 |

which... I guess is unrelated, but should still be fixed. Now, If I change the private methods/getters to public: I get this mess:

  var e, t, r
  function n(e, t, r) {(function (e, t) {if (t.has(e)) throw new TypeError("Cannot initialize the same private elements twice on an object")})(e, t), t.set(e, r)} function i(e, t) {
    var r = function (e, t, r) {
      if (!t.has(e)) throw new TypeError("attempted to " + r + " private field on non-instance")
      return t.get(e)
    }(e, t, "get")
    return function (e, t) {
      if (t.get) return t.get.call(e)
      return t.value
    }(e, r)
  } r = new WeakMap, e = class {
    constructor() {
      var e, i, a, l
      n(this, r, {writable: !0, value: "hi"}), e = this, i = "g", l = this, (a = t) && Object.defineProperty(e, i, {enumerable: a.enumerable, configurable: a.configurable, writable: a.writable, value: a.initializer ? a.initializer.call(l) : void 0})
    } get b() {return "hello" + i(this, r)} get c() {return this.b}
  }, a = e.prototype, l = "g", o = [f], u = {configurable: !0, enumerable: !0, writable: !0, initializer: null}, p = {}, Object.keys(u).forEach((function (e) {p[e] = u[e]})), p.enumerable = !!p.enumerable, p.configurable = !!p.configurable, ("value" in p || p.initializer) && (p.writable = !0), p = o.slice().reverse().reduce((function (e, t) {return t(a, l, e) || e}), p), d && void 0 !== p.initializer && (p.value = p.initializer ? p.initializer.call(d) : void 0, p.initializer = void 0), void 0 === p.initializer && (Object.defineProperty(a, l, p), p = null), t = p
  var a, l, o, u, d, p

which is way wrong.

kategengler commented 2 years ago

I was able to reproduce the ember output in babel-transpilation-tests by passing modules: false to preset-env (as ember-cli-babel does).

NullVoxPopuli commented 2 years ago

can we omit modules: false when building ember?

NullVoxPopuli commented 2 years ago

but also, what do modules have to do with class property transpilation? :thinking: maybe that's the babel bug?

kategengler commented 2 years ago

I have pretty much zero subject-matter knowledge here. From reading preset-env docs and from some commit history, it seems like cli-babel does it's own module stuff. No idea what the interaction between the modules and other transpilation could be.

NullVoxPopuli commented 2 years ago

as an experiment, I tried the following changes... to see if I can have any control over babel whatsoever... the answers appears to be no.

diff --git a/ember-cli-build.js b/ember-cli-build.js
index 48e94e9..b11292d 100644
--- a/ember-cli-build.js
+++ b/ember-cli-build.js
@@ -4,6 +4,9 @@ const EmberApp = require('ember-cli/lib/broccoli/ember-app');

 module.exports = function (defaults) {
   let app = new EmberApp(defaults, {
+    'ember-cli-babel': {
+      useBabelConfig: true,
+    },
     // Add options here
   });

the babel.config.js

 'use strict';

const { buildEmberPlugins } = require('ember-cli-babel');

module.exports = function (api) {
  api.cache(true);

  console.log('ugh'); // <-- I can see this in the terinal

  return {}; // <-- I only added this because nothing I changed below seemed to affect the build output. 
  // ^ I would expect this to break the build... or at the very least babel should complain about running in to decorators
  //   and not having the decorator plugin loaded...

  return {
    presets: [
      [
        require.resolve('@babel/preset-env'),
        {
          // modules: 'auto',
          // targets: {
          //   // esmodules: true,
          //   browsers: ['last 1 firefox versions'],
          // },
        },
      ],
    ],
    plugins: [
      [require.resolve('@babel/plugin-proposal-decorators'), { legacy: true }],
      // ...buildEmberPlugins(__dirname)
    ],
  };
};
kategengler commented 2 years ago

I retract my comment about modules: false, since I cannot reproduce it's effect again. I'm puzzled but /shrug

oliverlj commented 2 years ago

Moving targets from const browsers = ['last 1 Chrome versions', 'last 1 Firefox versions', 'last 1 Safari versions']; to const browsers = ['> 0.6%', 'not IE 11', 'not op_mini all', 'not dead'];

let me add private methods in classes.

NullVoxPopuli commented 2 years ago

Unfortunately, this doesn't solve my root issue:

class properties should not be transpiled away, babel is doing too much work

image

NullVoxPopuli commented 2 years ago

When debugging this locally, I discovered the following allows native class transpilation: image (a change to ember-cli-babel)

and in my linked ember-cli-babel, updating browserlist

❯  npx browserslist --update-db
Latest version:     1.0.30001278
Installed versions: 1.0.30001066, 1.0.30001196

it was a bit behind:

Target browser changes:
- and_chr 94
+ and_chr 95
- android 94
+ android 95
- chrome 91
- edge 93
- edge 92
- firefox 91
+ firefox 94
- opera 78
+ opera 81
NullVoxPopuli commented 2 years ago

This results in my output js having proper fields: image

and a file reduction of: standard new app, no actual code in it:

before ``` ❯ la dist/assets/ total 1.8M drwxrwxr-x 2 nullvoxpopuli nullvoxpopuli 12 Nov 6 12:00 . drwxrwxr-x 4 nullvoxpopuli nullvoxpopuli 7 Nov 6 12:00 .. -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 0 Oct 25 20:47 my-app.css -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 6.7K Nov 6 13:20 my-app.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 8.0K Nov 6 13:20 my-app.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 1.1K Nov 6 13:20 tests.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 1.4K Nov 6 13:20 tests.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 79K Nov 6 13:20 test-support.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 97K Nov 6 13:20 test-support.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 0 Nov 6 13:20 vendor.css -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 2.1M Nov 6 13:20 vendor.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 2.5M Nov 6 13:20 vendor.map ```
after ``` ❯ la dist/assets/ total 1.8M drwxrwxr-x 2 nullvoxpopuli nullvoxpopuli 12 Nov 6 12:00 . drwxrwxr-x 4 nullvoxpopuli nullvoxpopuli 7 Nov 6 12:00 .. -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 0 Oct 25 20:47 my-app.css -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 5.4K Nov 6 13:14 my-app.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 6.7K Nov 6 13:14 my-app.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 1.1K Nov 6 13:14 tests.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 1.4K Nov 6 13:14 tests.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 79K Nov 6 13:14 test-support.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 97K Nov 6 13:14 test-support.map -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 0 Nov 6 13:14 vendor.css -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 2.1M Nov 6 13:14 vendor.js -rw-rw-r-- 1 nullvoxpopuli nullvoxpopuli 2.5M Nov 6 13:14 vendor.map ```
NullVoxPopuli commented 2 years ago

More importantly, on a slightly bigger app (still smol): https://github.com/NullVoxPopuli/limber

ember build --environment=production

I don't notice any size differences. :thinking:

before hashes are all messed up cause I think my yarn linking changed something ``` - dist/assets/chunk.02266f327fc7017b4ee2.js: 3.31 KB (1.52 KB gzipped) - dist/assets/chunk.1c2dca68da24b8146913.js: 18.01 KB (7.22 KB gzipped) - dist/assets/chunk.3205c7a85e8f728a503f.js: 1.38 KB (796 B gzipped) - dist/assets/chunk.3ddd32b1200b6a36d63c.js: 41.3 KB (3.14 KB gzipped) - dist/assets/chunk.4ec1f6e9b206eeb40f46.js: 6.28 MB (1.52 MB gzipped) - dist/assets/chunk.a126d0fce8da2db3a3eb.js: 399.13 KB (136.79 KB gzipped) - dist/assets/chunk.a223687bc8b16f057675.js: 279 B (248 B gzipped) - dist/assets/chunk.a40f403ae3edfb05ef53.js: 436.29 KB (122.13 KB gzipped) - dist/assets/chunk.d741dbd9bf21cec24f34.js: 2.09 MB (484.15 KB gzipped) - dist/assets/chunk.de90bf08e38eaefbb4c0.js: 872.49 KB (277.83 KB gzipped) - dist/assets/chunk.f919ae8573d014be007b.js: 53.26 KB (14.61 KB gzipped) - dist/assets/limber.3af9ba98984fa602106f541342f0cc28.css: 18.33 KB (5.14 KB gzipped) - dist/assets/limber.css: 29.66 KB (7.91 KB gzipped) - dist/assets/vendor.47e5b13a6c66903c00db8ae984063008.css: 5.9 KB (1.4 KB gzipped) - dist/assets/vendor.f795a98317800a0f8697b832711ba6ae.js: 738.71 KB (207.25 KB gzipped) ```
after ``` - dist/assets/chunk.04aa101d202566b53b47.js: 872.48 KB (277.84 KB gzipped) - dist/assets/chunk.24a5190acb6459d3d808.js: 399.13 KB (136.79 KB gzipped) - dist/assets/chunk.44f3400873c359552005.js: 278 B (247 B gzipped) - dist/assets/chunk.54301774813e3fba5cad.js: 18.01 KB (7.23 KB gzipped) - dist/assets/chunk.6f64bdaebf690a50d2b1.js: 53.22 KB (14.61 KB gzipped) - dist/assets/chunk.aa6c5dca29c80d51b17e.js: 1.38 KB (799 B gzipped) - dist/assets/chunk.aa8f618009c59129c97b.js: 2.09 MB (484.15 KB gzipped) - dist/assets/chunk.c2d016a5c14f08de5643.js: 6.28 MB (1.52 MB gzipped) - dist/assets/chunk.cc3ca74a0910fec0d6c3.js: 41.3 KB (3.14 KB gzipped) - dist/assets/chunk.e2f6165688dcf150a33c.js: 3.31 KB (1.52 KB gzipped) - dist/assets/chunk.f9541f54f9cd27efcd21.js: 436.38 KB (122.03 KB gzipped) - dist/assets/limber.3af9ba98984fa602106f541342f0cc28.css: 18.33 KB (5.14 KB gzipped) - dist/assets/limber.css: 29.66 KB (7.91 KB gzipped) - dist/assets/vendor.47e5b13a6c66903c00db8ae984063008.css: 5.9 KB (1.4 KB gzipped) - dist/assets/vendor.f795a98317800a0f8697b832711ba6ae.js: 738.71 KB (207.25 KB gzipped) ```
kategengler commented 2 years ago

There are some very small changes, easier to see when sorted:

before ``` - dist/assets/chunk.4ec1f6e9b206eeb40f46.js: 6.28 MB (1.52 MB gzipped) - dist/assets/chunk.d741dbd9bf21cec24f34.js: 2.09 MB (484.15 KB gzipped) - dist/assets/chunk.de90bf08e38eaefbb4c0.js: 872.49 KB (277.83 KB gzipped) - dist/assets/vendor.f795a98317800a0f8697b832711ba6ae.js: 738.71 KB (207.25 KB gzipped) - dist/assets/chunk.a40f403ae3edfb05ef53.js: 436.29 KB (122.13 KB gzipped) - dist/assets/chunk.a126d0fce8da2db3a3eb.js: 399.13 KB (136.79 KB gzipped) - dist/assets/chunk.f919ae8573d014be007b.js: 53.26 KB (14.61 KB gzipped) - dist/assets/chunk.3ddd32b1200b6a36d63c.js: 41.3 KB (3.14 KB gzipped) - dist/assets/limber.css: 29.66 KB (7.91 KB gzipped) - dist/assets/limber.3af9ba98984fa602106f541342f0cc28.css: 18.33 KB (5.14 KB gzipped) - dist/assets/chunk.1c2dca68da24b8146913.js: 18.01 KB (7.22 KB gzipped) - dist/assets/vendor.47e5b13a6c66903c00db8ae984063008.css: 5.9 KB (1.4 KB gzipped) - dist/assets/chunk.02266f327fc7017b4ee2.js: 3.31 KB (1.52 KB gzipped) - dist/assets/chunk.3205c7a85e8f728a503f.js: 1.38 KB (796 B gzipped) - dist/assets/chunk.a223687bc8b16f057675.js: 279 B (248 B gzipped) ```
after ``` - dist/assets/chunk.c2d016a5c14f08de5643.js: 6.28 MB (1.52 MB gzipped) - dist/assets/chunk.aa8f618009c59129c97b.js: 2.09 MB (484.15 KB gzipped) - dist/assets/chunk.04aa101d202566b53b47.js: 872.48 KB (277.84 KB gzipped) - dist/assets/vendor.f795a98317800a0f8697b832711ba6ae.js: 738.71 KB (207.25 KB gzipped) - dist/assets/chunk.f9541f54f9cd27efcd21.js: 436.38 KB (122.03 KB gzipped) - dist/assets/chunk.24a5190acb6459d3d808.js: 399.13 KB (136.79 KB gzipped) - dist/assets/chunk.6f64bdaebf690a50d2b1.js: 53.22 KB (14.61 KB gzipped) - dist/assets/chunk.cc3ca74a0910fec0d6c3.js: 41.3 KB (3.14 KB gzipped) - dist/assets/limber.css: 29.66 KB (7.91 KB gzipped) - dist/assets/limber.3af9ba98984fa602106f541342f0cc28.css: 18.33 KB (5.14 KB gzipped) - dist/assets/chunk.54301774813e3fba5cad.js: 18.01 KB (7.23 KB gzipped) - dist/assets/vendor.47e5b13a6c66903c00db8ae984063008.css: 5.9 KB (1.4 KB gzipped) - dist/assets/chunk.e2f6165688dcf150a33c.js: 3.31 KB (1.52 KB gzipped) - dist/assets/chunk.aa6c5dca29c80d51b17e.js: 1.38 KB (799 B gzipped) - dist/assets/chunk.44f3400873c359552005.js: 278 B (247 B gzipped) ```
NullVoxPopuli commented 2 years ago

It seems there is different behavior in production builds :thinking: I've yet to figure out how to retain class property assignment in production builds.

For example, this same app, but in a development build. (now with sorted sizes, thanks!!!!)

ember build --environment="development"
Before ``` ❯ la dist/assets/ --sort size total 8.7M 9.3M Nov 6 21:18 chunk.492823d7f6cf487a4f95.js 3.7M Nov 6 21:18 chunk.25a83d3433a76f959c91.js 3.1M Nov 6 21:18 vendor.map 2.6M Nov 6 21:18 vendor.js 1.7M Nov 6 21:18 chunk.278c24c6a5246e31c3d5.js 1.4M Nov 6 21:18 chunk.a4d40ba6cdb184cd22c6.js 661K Nov 6 21:18 chunk.039933d153d921b042ce.js 577K Nov 6 21:18 bundle.html 548K Nov 6 21:18 chunk.5561052f2fffe029e4fb.js 223K Nov 6 21:18 chunk.7e8165ef2799e8cb92be.js 78K Nov 6 21:18 test-support.map 64K Nov 6 21:18 test-support.js 56K Nov 6 21:18 chunk.c6c5c880c00af7a96495.js 54K Nov 6 21:18 chunk.78a3dd08f2783e7bf509.js 42K Nov 6 21:18 chunk.424d14eef482d6a5d3db.js 30K Nov 6 21:18 limber.css 15K Nov 6 21:18 chunk.ec17c079b01f5944f895.js 14K Nov 6 21:18 test-support.css.map 11K Nov 6 21:18 vendor.css.map 9.8K Nov 6 21:18 test-support.css 8.0K Nov 6 21:18 chunk.2573767c438da8ce1b29.js 7.7K Nov 6 21:18 vendor.css 5.7K Nov 6 21:18 chunk.4fdb358b58ce70a4d904.js 1.1K Nov 6 21:18 chunk.61786fecb26d0f954064.js ```
After ``` total 8.7M 9.3M Nov 6 21:20 chunk.d86d10af1c7b14d36937.js 3.7M Nov 6 21:20 chunk.3f85e9e9f4b1c57739fa.js 3.1M Nov 6 21:20 vendor.map 2.6M Nov 6 21:20 vendor.js 1.7M Nov 6 21:20 chunk.e5165ddeebc1770758cf.js 1.4M Nov 6 21:20 chunk.6fd23a797bac42050508.js 661K Nov 6 21:20 chunk.6c3eb8b409ae91a12db3.js 578K Nov 6 21:20 bundle.html 548K Nov 6 21:20 chunk.5561052f2fffe029e4fb.js 216K Nov 6 21:20 chunk.c09b98e94353dca684df.js 78K Nov 6 21:20 test-support.map 64K Nov 6 21:20 test-support.js 56K Nov 6 21:20 chunk.30310909bcf7d86205a7.js 50K Nov 6 21:20 chunk.32fa93bd9e0c842652ad.js 42K Nov 6 21:20 chunk.424d14eef482d6a5d3db.js 30K Nov 6 21:20 limber.css 15K Nov 6 21:20 chunk.7ef78ff81928141e6bcc.js 14K Nov 6 21:20 test-support.css.map 11K Nov 6 21:20 vendor.css.map 9.8K Nov 6 21:20 test-support.css 7.7K Nov 6 21:20 vendor.css 7.3K Nov 6 21:20 chunk.1b63aa140ffb6736f75b.js 5.7K Nov 6 21:20 chunk.35afd689835f1817a99f.js 1.1K Nov 6 21:20 chunk.d28e8c4f64f415c7f13e.js ```

These sizes are more noticable anyway -- which is good, because anything I can do to help make development a bit faster, the better.

Most meaningful changes in these diffs:

I looked aver the readme again and noticed that there is a

babel: {
  loose: true
}

option available, but it seems that's only used for addons, not apps.

my diff here seems to be the only way to allow class properties to not be compiled.

diff --git a/lib/babel-options-util.js b/lib/babel-options-util.js
index ff49d6e..c3469ea 100644
--- a/lib/babel-options-util.js
+++ b/lib/babel-options-util.js
@@ -346,7 +346,7 @@ function _addDecoratorPlugins(plugins, options, config, parent, project) {
       plugins,
       [
         require.resolve("@babel/plugin-proposal-class-properties"),
-        { loose: options.loose || false },
+        { loose: true || options.loose || false },
       ],
       _buildClassFeaturePluginConstraints(
         {

gonna try a bigger app - maybe the ember-website

NullVoxPopuli commented 2 years ago

ok, so I decided to go with https://github.com/emberobserver/client

one file (the app's js file) has ~ 2.7% smaller. (development)

NullVoxPopuli commented 2 years ago

so... I guess... do we ever not want loose mode? Is something deeper wrong here? in my babel-only repro loose mode was not needed :thinking: (nor was even specifying the class fields plugin) :shrug:

rwjblue commented 2 years ago

Presumably this is the same as https://github.com/babel/ember-cli-babel/issues/419#issuecomment-971787156