(feat): closure compiler for minification

[ambroseus]

Related to #358
(for internal use only :)
Added ability to use closure compiler instead of terser plugin for code minification

"build": "tsdx build --closureCompiler"

to play with advanced options use tsdx.config.js

const closureCompiler = require('@ampproject/rollup-plugin-closure-compiler');

const closureCompilerPlugin = closureCompiler({
  compilation_level: 'ADVANCED_OPTIMIZATIONS',
});

module.exports = {
  rollup(config) {
    config.plugins = config.plugins.map(plugin => {
      // override closure compiler plugin's default config
      if (plugin && plugin.name === closureCompilerPlugin.name) {
        return closureCompilerPlugin;
      }
      return plugin;
    });

    return config;
  },
};

[agilgur5]

Left some comments. microbundle has a similar discussion in developit/microbundle#569 which seems to have some bugs(?) but behind an experimental flag or something it could be ok

[ambroseus]

@agilgur5 please review:
--closureCompiler CLI flag
closureComplierOptions section in tsdx.config.js

[agilgur5]

Thanks for making the changes @ambroseus .

I've left a few comments here, but the key concepts:

• I don't think we want to open up the tsdx.config.js API just yet, that needs some more thought process. Also may have misinterpreted my previous comments -- I meant to just add the config you had as a default and can customize through existing config.plugins style.
• The tests right now don't test if Closure was used instead of Terser, they just check that setting the --closureCompiler flag (and configuring Closure) doesn't fail. I'm fine with leaving out tests right now since this is experimental, so maybe just delete them.
  • I'm not sure what tests for this should look like. Maybe snapshots are appropriate (like microbundle did) though I'm not a big fan of them.
  • No need for a new fixture directory if tests are added, can re-use build-default if just using the flag, and can use build-withConfig for configuring it (though that's not yet set-up for automated testing). Maybe configuring it and ensuring the configuration succeeded is an appropriate test. But I'm not sure how we'd do that without a snapshot either

[agilgur5]

Yea, I think it's fine to leave tests out for now, don't want to add too much iteration for you since I'm not really sure myself what those should look like and build-withConfig isn't yet set-up for automated tests. Feel free to share any ideas on that front though

[ambroseus]

@agilgur5 so... may be my first approach with dedicated closureCompileOptions config file and env var instead of --closureCompiler flag is more applicable considering this is INTERNAL UNSTABLE VERY SECRET feature? ;)

[jaredpalmer]

I think snapshots on code output make sense here for testing. It may be hard to understand prod output. While it’s not exactly an integration test, perhaps we could run prettier on the prod output of the test before the snapshot so it’s at least readable.

While this experimental, if it works, I can’t see why we wouldn’t use it in core. IIRC React core uses it.

A plugin system is beyond the scope of this issue, but a good one. My 2cents on this, is that unlike Gatsby who’s core does almost nothing, TSDX’s core should be the happy path and cover 80-90% of use cases.

[agilgur5]

@agilgur5 so... may be my first approach with dedicated closureCompileOptions config file and env var instead of --closureCompiler flag is more applicable considering this is INTERNAL UNSTABLE VERY SECRET feature? ;)

Well I don't think it's "very secret" given this public PR haha 😝
But I think opening up two new, different API surfaces (a dedicated config file and an env var) is much more confusing than just adding a flag.
And I'm totally fine with getting feedback from the community on an "unstable experimental" feature, it doesn't have to be internal-only. This is the same way React and Rollup have incrementally released features too, though usually they'll initially prefix them as unstable_ or experimental before.

It can be configured in tsdx.config.js with config.plugins already, so let's just use the same method for now.

While this experimental, if it works, I can’t see why we wouldn’t use it in core.

Based on microbundle's work in developit/microbundle#569 and @ambroseus's testing in #358, it sounds like it's not always better than Terser (but often is?). ADVANCED_OPTIMIZATIONS might also take longer or use more memory than Terser, but it wasn't clear to me if it necessarily did.

A plugin system is beyond the scope of this issue, but a good one.

I still need to write up an RFC for this but I have it in my head. The plugin system would be more to make common use cases of tsdx.config.js easier. Because they often require additional dependencies or don't apply to everyone, it makes sense for it to be out of core, but perhaps still maintained internally for better maintainability against changes to the internal configs (and because they're common).
There's also a separate use of the word "plugin" in that a good bit of TSDX could probably be re-written as a Rollup plugin(s)

[ambroseus]

@agilgur5 @jaredpalmer please, review

there are some closure-compiler specific tests. only CC can minify this:

import { split } from './foo';

export const sum = (a: number, b: number) => {
  return a + b;
};

const bar = split('bar');

export const signature = `${split('bar').join('')} ${sum(bar.length, -3)}`;

to this:

var signature="bar 0";function sum(a,b){return a+b};export{signature,sum}

terser's output to compare:

var n=function(n){return n.split("")},r=function(n,r){return n+r},t=n("bar"),
e=n("bar").join("")+" "+r(t.length,-3);export{e as signature,r as sum};

[agilgur5]

Great work on making a closure compiler specific test!! Would be great to get another test that only ADVANCED_OPTIMIZATIONS can do if it's not too difficult/involved.

I left some comments on a few small things, but more generally I still don't think we should add a closureCompilerOptions section yet, that's something we can think about/discuss in the future, possibly in my plugins RFC. I added a suggestion to the tsdx.config.closure-advanced.js for what to use for this for now.

I'm also going to move the existing manual tests that are in the withConfig fixture to an integration tests set-up, I'll rebase this after so that the code that's already here isn't deleted, but moved instead. Don't worry about doing anything for this, it's just a step I'll need to do before merging.

[agilgur5]

is there another simple test we can add to this that only ADVANCED_OPTIMIZATIONS can do?

[ambroseus]

@agilgur5 next round), please review

[agilgur5]

Just some minor things around explicitness, comments, and style. All the major pieces are in line!!

[ambroseus]

@agilgur5 final (maybe:) round, please review

[agilgur5]

Ok, this is basically good to go. Just one q on the patterns in-line

There's a few nits here, but I can edit those myself as this needs to get rebased and updated to the new testing set-up anyway.

[agilgur5]

const

[agilgur5]

const

[agilgur5]

huh... doesn't this minification break the usage?? since signature is no longer exported. is that a bug in closure or is there a way to disable exports minification?

[ambroseus]

@agilgur5 hmm.. this is not good.. need to investigate (a lot of ADVANCED options)

[agilgur5]

Ah, I haven't looked the API at all. Keywords would likely be "exports" and "mangle" as I believe the technical term for changing variable names is "mangling"

At least we're already iterating on some future defaults 😅

[agilgur5]

While this is waiting for more changes, figure I should credit you in allcontributors for your contributions especially since a piece of the code here went into #627 already.

[agilgur5]

@allcontributors please add @ambroseus for test, examples, questions, ideas

[allcontributors]

@agilgur5

I've put up a pull request to add @ambroseus! 🎉

[ambroseus]

@agilgur5 thanks! proud to work with tsdx team and you personally :)

[ambroseus]

@agilgur5 rebased & moved tests to integration folder. please check)

[agilgur5]

There's a few bugs that came in from merge conflict resolution that need to be fixed.

A few of the remaining comments from last review haven't been resolved yet, like const instead of let, and most notably the exports.a name mangling with ADVANCED_OPTIMIZATIONS. That one's the big blocker here. Any other changes I could quickly fix myself to push this out, but that one's more involved and part of the core work you've been doing. So focus your time on that as first priority.

---

integration is also the wrong directory to put this 😅 This isn't an integration test as Closure Compiler is being added as a feature, not an external integration. It would go into e2e.
But as I said before, don't worry too much about that, I can make those changes myself when this is ready as the test structure and organization changed quite a bit and I'm much more familiar with that (there's also some issues here with beforeEach, let instead of const, expect(output.code) that could be placed differently, etc). Please focus on the other changes above.

Thanks for iterating on this and trying to tackle that mess of a rebase yourself!

[ambroseus]

@agilgur5 ... time is going) I thought a lot about this feature and decide to close PR, because it's not very useful right now. We can implement it later as tsdx plugin for ex.