Cleanup how optimize streams work, in preparation for babel stream #524

FredKSchott · 2016-12-20T01:06:01Z

This PR addresses two problems:

All the build options had gotten pretty confusing, and assumed a world where each language only had one optimizer. This PR cleans up the CLI commands and simplified the optimize options that the main build function accepts.
Constructing our optimize stream inside of pipes and gulp if's was too limiting. We now call out to getOptimizeStreams to calculate and return the optimize streams that we need.

This PR should be a noop functionality-wise, and prepares for a much easier diff in #525.

justinfagnani · 2016-12-20T21:41:04Z

src/build/build.ts

+    polymerProject.splitHtml(),
+    getOptimizeStreams(optimizeOptions),
+    polymerProject.rejoinHtml()
+  ]).reduce((a: NodeJS.ReadableStream, b: NodeJS.ReadWriteStream) => {


Let's extract this into a named function that self-documents:

import {pipeStreams} from './streams'; const sourcesStream = pipeStreams([ polymerProject.sources(), polymerProject.splitHtml(), ]);

justinfagnani · 2016-12-20T21:41:55Z

src/build/build.ts

+    polymerProject.splitHtml(),
+    getOptimizeStreams(optimizeOptions),
+    polymerProject.rejoinHtml()
+  ]).reduce((a: NodeJS.ReadableStream, b: NodeJS.ReadWriteStream) => {


justinfagnani · 2016-12-20T21:43:33Z

src/build/optimize-streams.ts

+ * OptimizeOptions given.
+ */
+export function getOptimizeStreams(optimizeOptions: OptimizeOptions) {
+  let streams = [];


justinfagnani · 2016-12-20T22:20:56Z

src/build/optimize-streams.ts

@@ -30,6 +30,11 @@ export type FileCB = (error?: any, file?: File) => void;
 export type CSSOptimizeOptions = {
  stripWhitespace?: boolean;
 };
+export interface OptimizeOptions {
+  htmlMinify?: HTMLMinifierOptions;


Why not have the nested structure here that we do in BuildOptions? Then in #525 instead of adding a jsCompile property, you add compile to the js property. Or to go further, just get rid of this interface and pass BuildOptions to getOptimizeStreams.

I wanted to pass this subset of build options to getOptimizeStreams() to simplify the interface. But I can match the nested structure from BuildOptions if you prefer.

Nice, nested looks much cleaner (less mapping and plays much nicer with TS)

justinfagnani · 2016-12-20T22:23:57Z

src/commands/build.ts

@@ -32,6 +32,21 @@ export class BuildCommand implements Command {

  args = [
    {
+      name: 'js-minify',


Not so sure about this change. The reason for namespacing flags like html.collapseWhitespace was to at some point be able to easily map flags to build options. Continuing that pattern would lead to js.minify, css.minify and html.minify.

I really dislike the . in the command line flag, a dash feels much more natural and doesn't break the expected CLI dash-case naming convention. We can still map easily regardless of . or -.

I don't think it's that uncommon to use . when flags map to JSON-like structures. Mapping with - is harder: does foo-bar-baz map to foo.bar.baz, foo.bar-baz, etc.

But the user will never know about or use that JSON-like structure, right? If we were maintaining a large set of rules or mapping to an interface that the user was familiar with I would agree, but I don't see the point in breaking our expected naming system by mapping to an internal interface that could change at any time.

The user probably will, when we explain that the flags map to the options in polymer.json

Ah, I hadn't assumed we'd use the same options for build() in polymer.json.

Can we table this decision then until we start on the polymer.json options? I think that will be a more productive conversation when those options actually exist vs. planning for something we haven't really thought through yet (at least I haven't yet).

We can, but the pattern had been set up with that goal in mind. If we defer the discussion can we stick with the current pattern?

~~sure thing, rolling back~~
discussed and planned out in person, our flags will use this format (https://docs.google.com/document/d/1mD1cVhxdxY-bUsGBX44anC1875Afx7UHue4uRCNVc4o). Updating the BuildOptions to match the planned polymer.json options now.

FredKSchott · 2016-12-21T00:06:21Z

@justinfagnani comments addressed, PTAL

justinfagnani

Mostly fine, though we're changing the options available to pipe through to the minifiers. I had thought that we wanted to expand these options. I want to be sure we're on the right path there.

justinfagnani · 2016-12-21T19:21:43Z

src/commands/build.ts

@@ -44,11 +59,6 @@ export class BuildCommand implements Command {
          'additional `<link rel="prefetch">` tags into ' +
          'entrypoint and `<link rel="import">` tags into fragments and shell.'
    },
-    {
-      name: 'html.collapseWhitespace',


can we deprecate for one release before removing?

Actually, collapseWhitespace is only one of many options offered here: https://www.npmjs.com/package/html-minifier

Are we now mapping minify to a specific set of those?

Yes, see: https://github.com/Polymer/polymer-cli/pull/524/files#diff-6efc8ba30392e1492e440aea247b32f6R155

With the way command-line-args works, anyone who is using this will get an error once this is removed. That feels like enough to me, wdyt? Are enough people actually using this?

In regards to expanding vs. shrinking these options, we're simply adding a flag to turn a default minify on/off. This doesn't prevent us from supporting custom minify options via this same flag later down the road. (Unless you're suggesting we continue the current scheme with a flag for every possible option). The intention here is to bring us closer to supporting cli options and polymer.json options, not farther away.

Again, I don't think we should spend too much time going back and forth on how these CLI flags will match the polymer.json interface, when that interface doesn't even exist yet. I'd much rather discuss that when we do add the polymer.json interface, either in a design proposal or the PR itself. This won't be going out to users until then.

FredKSchott · 2016-12-28T19:27:45Z

@justinfagnani PTAL

FredKSchott · 2017-01-12T22:39:29Z

@justinfagnani code now matches our new design doc, PTAL

justinfagnani

LGTM! Please squash :)

* Convert build to single build stream (#509) * Convert build to single build stream, add bundled flag * Clarify `--add-prefetch` build flag (#510) * rename confusing --insert-dependency-links flag to --prefetch-dependencies * rename prefetchDepedencies to addPrefetchLinks * rename to insert-prefetch-links * respond to review comments * Cleanup how optimize streams work, in preparation for babel stream (#524) * Cleanup how optimize streams work, in preparation for babel stream * death to lets * redeisn optimize interface * pull stream piping into a helper function * rollback cli dash-case naming * remove unused gulpif * update buildoptions to match planned polymer.json options * add babel to build stream, replace uglify with babili es2015 minifier (#525) * Cleanup how optimize streams work, in preparation for babel stream * add babel to build stream * death to lets * redeisn optimize interface * pull stream piping into a helper function * rollback cli dash-case naming * replace uglify with babili * fix bad merge conflict * fixup js-minify description * remove unused gulpif * update buildoptions to match planned polymer.json options * remove old comment * let to const * add service worker flag to build command (#534) * add service worker flag to build command * reword flags and descriptions * quick reword again * get rid of skiplibcheck * debug to info * Add support for polymer.json build configurations (#545) * Add support for named build permutations * respond to PR feedback * add comments on build function * update logging to be cleaner * update for new naming rules * update polymer-project-config to v1.2.0 * Update CHANGELOG.md * update polymer-build version, update shrinkwrap

Cleanup how optimize streams work, in preparation for babel stream

0774cec

FredKSchott assigned justinfagnani Dec 20, 2016

FredKSchott mentioned this pull request Dec 20, 2016

add babel to build stream, replace uglify with babili es2015 minifier #525

Merged

justinfagnani reviewed Dec 20, 2016

View reviewed changes

Fred K. Schott added 4 commits December 20, 2016 14:34

death to lets

1d8868f

redeisn optimize interface

dc7568f

pull stream piping into a helper function

5834a14

rollback cli dash-case naming

6a3d803

justinfagnani reviewed Dec 21, 2016

View reviewed changes

FredKSchott changed the base branch from master to new-build-flags January 9, 2017 22:46

Fred K. Schott added 3 commits January 9, 2017 14:50

Merge branch 'new-build-flags' into build-optimize-cleanup

3d741b6

remove unused gulpif

c63c4ce

update buildoptions to match planned polymer.json options

2f4a91d

justinfagnani approved these changes Jan 12, 2017

View reviewed changes

FredKSchott merged commit d384d7e into new-build-flags Jan 13, 2017

FredKSchott mentioned this pull request Mar 9, 2017

Merge New Build Permutation Support #551

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup how optimize streams work, in preparation for babel stream #524

Cleanup how optimize streams work, in preparation for babel stream #524

FredKSchott commented Dec 20, 2016 •

edited

Loading

justinfagnani Dec 20, 2016

FredKSchott Dec 20, 2016

justinfagnani Dec 20, 2016

justinfagnani Dec 20, 2016

justinfagnani Dec 20, 2016

FredKSchott Dec 20, 2016

FredKSchott Dec 20, 2016

justinfagnani Dec 20, 2016

FredKSchott Dec 20, 2016

justinfagnani Dec 20, 2016

FredKSchott Dec 20, 2016

justinfagnani Dec 20, 2016

FredKSchott Dec 20, 2016

justinfagnani Dec 20, 2016

FredKSchott Dec 21, 2016 •

edited

Loading

FredKSchott commented Dec 21, 2016

justinfagnani left a comment

justinfagnani Dec 21, 2016

justinfagnani Dec 21, 2016

FredKSchott Dec 22, 2016

FredKSchott Dec 22, 2016 •

edited

Loading

FredKSchott commented Dec 28, 2016

FredKSchott commented Jan 12, 2017

justinfagnani left a comment

Cleanup how optimize streams work, in preparation for babel stream #524

Cleanup how optimize streams work, in preparation for babel stream #524

Conversation

FredKSchott commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

FredKSchott commented Dec 21, 2016

justinfagnani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FredKSchott Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

FredKSchott commented Dec 28, 2016

FredKSchott commented Jan 12, 2017

justinfagnani left a comment

Choose a reason for hiding this comment

FredKSchott commented Dec 20, 2016 •

edited

Loading

FredKSchott Dec 21, 2016 •

edited

Loading

FredKSchott Dec 22, 2016 •

edited

Loading