(feat): closure compiler for minification by ambroseus · Pull Request #525 · jaredpalmer/tsdx

ambroseus · 2020-02-28T11:45:27Z

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 · 2020-02-29T08:45:37Z

@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 · 2020-03-11T07:15:42Z

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 · 2020-03-11T09:42:02Z

@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 · 2020-03-11T11:53:15Z

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 · 2020-03-12T03:53:13Z

@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 · 2020-03-14T20:30:32Z

@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 · 2020-03-14T20:48:07Z

+    output = shell.grep('bar 0', [
+      'dist/build-withconfig.esm.production.min.js',
+    ]);
+    expect(/bar 0/.test(output.stdout)).toBeTruthy();


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

…x.config.js

ambroseus · 2020-03-20T11:11:51Z

@agilgur5 next round), please review

agilgur5

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

ambroseus · 2020-03-21T12:21:26Z

@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 · 2020-03-29T05:00:10Z

+  it('should minify bundle with default options', () => {
+    util.setupStageWithFixture(stageName, 'build-withConfig');
+
+    let output = shell.exec('node ../dist/index.js build --closureCompiler');


agilgur5 · 2020-03-29T05:00:24Z

+    util.setupStageWithFixture(stageName, 'build-withConfig');
+    shell.mv('-f', 'tsdx.config.closure-advanced.js', 'tsdx.config.js');
+
+    let output = shell.exec('node ../dist/index.js build --closureCompiler');


agilgur5 · 2020-03-29T05:04:07Z

+const esmBundle = 'dist/build-withconfig.esm.js';
+
+const simplePattern = /exports\.signature=signature/;
+const advancedPattern = /exports\.a="bar 0"/;


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?

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

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 · 2020-04-08T02:48:44Z

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 · 2020-04-08T02:55:12Z

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

allcontributors · 2020-04-08T02:55:23Z

@agilgur5

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

ambroseus · 2020-04-08T04:41:02Z

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

ambroseus · 2020-04-08T10:45:08Z

@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 · 2020-05-27T05:29:39Z

@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.

agilgur5 reviewed Feb 28, 2020

View reviewed changes

Comment thread src/createRollupConfig.ts Outdated

Comment thread src/createRollupConfig.ts Outdated

Comment thread src/createRollupConfig.ts Outdated

ambroseus changed the title ~~Feature: closure compiler for minification~~ (feat): closure compiler for minification Mar 9, 2020

agilgur5 requested changes Mar 11, 2020

View reviewed changes

ambroseus force-pushed the feature/closure-compiler branch from a7832c5 to f22343b Compare March 14, 2020 11:18

ambroseus requested a review from agilgur5 March 14, 2020 20:24

agilgur5 reviewed Mar 17, 2020

View reviewed changes

agilgur5 mentioned this pull request Mar 17, 2020

(refactor): split build tests into separate files per fixture #621

Merged

agilgur5 reviewed Mar 19, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

agilgur5 reviewed Mar 19, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

agilgur5 mentioned this pull request Mar 19, 2020

tsdx-sass -- @rollup/plugin-image doesn't work in tsdx.config.js #628

Closed

ambroseus added 9 commits March 20, 2020 08:35

feature: minification with closure compiler

a17e146

feature: use closure-compiler.config.js

687eeff

use --closureCompiler CLI flag, closureCompilerOptions section in tsd…

32723eb

…x.config.js

remove compilerConfig path from constants

689d5a1

rebased with master

2148216

remove build-compiler fixture

1cde0c7

added tests with fixture build-withConfig

415e78d

closure-compiler specific tests

f46c04c

review fixes

11c5862

ambroseus force-pushed the feature/closure-compiler branch from 60c6a28 to 11c5862 Compare March 20, 2020 11:10

ambroseus commented Mar 20, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

ambroseus commented Mar 20, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

ambroseus commented Mar 20, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

improve tests

8ccba4d

agilgur5 reviewed Mar 21, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

agilgur5 reviewed Mar 21, 2020

View reviewed changes

Comment thread test/tests/tsdx-build-closure-compiler.test.js Outdated

agilgur5 reviewed Mar 21, 2020

View reviewed changes

Comment thread test/fixtures/build-withConfig/src/index.ts Outdated

Comment thread test/fixtures/build-withConfig/src/index.ts Outdated

Comment thread src/createRollupConfig.ts Outdated

improvements after review

557d8db

This was referenced Mar 23, 2020

[RFC] Shareable Plugins (for TSDX Build/Config, i.e. tsdx.config.js or Rollup in general) #635

Closed

Add Integration Tests #627

Merged

agilgur5 linked an issue Mar 26, 2020 that may be closed by this pull request

Add closure compiler #358

Closed

agilgur5 added this to the v0.13.x milestone Mar 26, 2020

agilgur5 mentioned this pull request Mar 28, 2020

(refactor): migrate all tests and test helper code to TS #649

Merged

agilgur5 approved these changes Mar 29, 2020

View reviewed changes

allcontributors Bot mentioned this pull request Apr 8, 2020

docs: add ambroseus as a contributor #670

Merged

ambroseus added 2 commits April 8, 2020 07:50

fixed conflicts

9878d56

moved build with closure tests to integration dir

7df73cd

agilgur5 requested changes Apr 13, 2020

View reviewed changes

Comment thread src/env.d.ts

Comment thread yarn.lock

Comment thread test/integration/tsdx-build-withClosure.test.ts

ambroseus marked this pull request as draft May 27, 2020 05:31

ambroseus closed this May 27, 2020

ambroseus deleted the feature/closure-compiler branch May 27, 2020 05:35

agilgur5 added kind: feature New feature or request solution: unresolved Issue has been closed by OP but root cause has not necessarily been resolved labels Aug 23, 2020

agilgur5 modified the milestones: v0.13.x - Template Improvements & More, Future Non-breaking Oct 22, 2020

Conversation

ambroseus commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilgur5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambroseus commented Feb 29, 2020

Uh oh!

agilgur5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agilgur5 commented Mar 11, 2020

Uh oh!

ambroseus commented Mar 11, 2020

Uh oh!

jaredpalmer commented Mar 11, 2020

Uh oh!

agilgur5 commented Mar 12, 2020

Uh oh!

ambroseus commented Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

agilgur5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

agilgur5 Mar 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambroseus commented Mar 20, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

agilgur5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ambroseus commented Mar 21, 2020

Uh oh!

agilgur5 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

agilgur5 Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

agilgur5 Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

agilgur5 Mar 29, 2020

Choose a reason for hiding this comment

Uh oh!

ambroseus commented Feb 28, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading

ambroseus commented Mar 14, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading

agilgur5 Mar 29, 2020 •

edited

Loading

agilgur5 left a comment •

edited

Loading