refactor: split `cache` and `parallel` options #108

alexander-akait · 2017-08-08T17:12:55Z

Notable Changes

Separate cache and parallel options, because they should not be tied up. I can enable cache but disable parallel, also i can enable parallel but disable cache. Seems we were not too careful when we were accepted PR with parallel option.
Fix [v1.0.0-beta.2] parallelization isn't working #97, options parallel doesn't work as expected ([v1.0.0-beta.2] parallelization isn't working #97).
A lot of tests (cache, parallel, ecma-8, ecma-7, ecma-6 ecma-5). Remove unnecessary extra tests from parallel tests.
Refactor some properties and method to easy read.
Simplify: remove src/uglify/cache.js, use cacache directly instead.
Don't use cacheIdentifier (aka metadata) field for invalidate cache because metadata should be used for other purposes (example metadata https://github.com/zkat/cacache#example-2). For invalidate cache we should use cacheKey.
Invalidate cache when uglify-es was updated.
Invalidate cache when uglifyjs-webpack-plugin was updated.
Invalidate cache when options for plugin was changed (we can't use only uglifyOptions because we can change extractComments or warningsFilter options). Maybe we can exclude some options not affecting on cache (test, include, exclude, parallel and sourceMap). Awaiting feedback.

Issues

One strange behavior

I remove [chunkhash] in ecma.test.js because webpack generate difference [chunkhash] for ecma-5 on node 4 and 8, I don't know why :/

michael-ciniawsky

👍 Only unrelated nits and one question :)

michael-ciniawsky · 2017-08-08T18:42:42Z

src/uglify/index.js

@@ -1,10 +1,9 @@
 import os from 'os';
+import cacache from 'cacache';
 import findCacheDir from 'find-cache-dir';


findCacheDir => cacheDir

cacheDir sounds like the result of findCacheDir

@Kovensky @michael-ciniawsky let's rename, thanks!

@Kovensky It is a {Function} which 'contains' the value =>

Directly pass as an argument with 'higher order' => const cache = (data, cacheDir(options)) => cache

Assign it to a scope/block/'namespace' instead of an 'redundant' variable

this.cacheDir = cacheDir(options) // ok const cache = { data, dir: cacheDir(options) } // ok const cacheDir = findCacheDir(options) // unnecessary :D

Anyways just a nitpick 🐦 😛

michael-ciniawsky · 2017-08-08T18:43:01Z

src/uglify/index.js

@@ -1,10 +1,9 @@
 import os from 'os';
+import cacache from 'cacache';


cacache => cache

@michael-ciniawsky i think rename can be misleading, we can use cache variable for other purpose. Also i think it is bad practice, but if someone from the reviewers considers renaming a good practice, say it here and I do it 😄

I'd suggest that if it's renamed, we don't go with cache as that's used all over the place in webpack already which may / may not be confusing.

I would stay with cacache as name. name is usual equal to package name.

michael-ciniawsky · 2017-08-08T18:46:33Z

test/uglify/cache.test.js

@@ -1,30 +0,0 @@
-import crypto from 'crypto';


Is the caching sufficiently tested elsewhere in the tests ?

@michael-ciniawsky be good add more tests, but cache.test.js doesn't test caching (just test get and put function)

let's add more tests

alexander-akait · 2017-08-16T21:09:24Z

@michael-ciniawsky @hulkish @bebraw @d3viant0ne friendly ping
Sorry for many changes, but current version don't works as expected 😞
After merge this PR i will do:

Refactor tests (now we got Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL. error if something unexpected in tests, but should get pretty output about errors)
Implement include and exclude options.

michael-ciniawsky · 2017-08-16T21:56:04Z

Refactor tests (now we got Timeout - Async callback was not invoked within timeout specified by jasmine.DEFAULT_TIMEOUT_INTERVAL. error if something unexpected in tests, but should get pretty output about errors)

This happend for me locally when I didn't build the src before running the tests (shouldn't be needed)

michael-ciniawsky

options.parallel.cache => options.cache is a breaking change 😞

alexander-akait · 2017-08-17T09:15:30Z

@michael-ciniawsky we have beta and can change this 😄

michael-ciniawsky · 2017-08-17T13:08:50Z

Yeah it's not optimal, but the cache as standalone option definitely makes more sense, let's break early as long as we basically 'can' 😛 . I review in depth later today

joshwiens · 2017-08-21T11:53:13Z

For reference, we can publish alpha / beta builds directly from this branch. Once everyone is satisfied (enough) with it, we can push a beta for the next semver:Major & get Sean to put it out on blast via Twitter.

That has the side effect of making it easier to test the changes in the major integrations ( @angular/cli & create-react-app )

michael-ciniawsky · 2017-09-12T14:09:59Z

Can we merge this PR and work on a solution for #120 ? Is it possible to rebase the current beta into a develop branch and 'revert' master to v0.4.6 102c18b since v1.0.0 will take a while (webpack v4.0.0 ?)[Particularly talking about npm releases]. Merging into master was a mistake of mine here since v0.4.6 would badly need a fix for the flanky postinstall script and still ships with webpack atm 😞 . With develop (@beta) && master (@latest) development doesn't get blocked by current ecosystem needs and beta dev can happen with less outside pressure + in the end unwanted breaking changes can be (easily) reverted before merging into master

Branch (Github)	Tag (npm)	SemVer
`develop`	`@beta`	🏷️ Major
`master`	`@latest`	🏷️ Minor/Patch
`release/vX.X.X`	`@X.X.X`	🏷️ Patch (only critical, generic (non use case specific)
`release/vX.X.X - 1`	`@X.X.X - 1`	EOL

alexander-akait · 2017-09-12T14:22:50Z

@d3viant0ne @hulkish friendly ping

sokra

I didn't check the tests, but changes make sense to me

sokra · 2017-09-29T14:36:33Z

src/uglify/index.js

@@ -1,10 +1,9 @@
 import os from 'os';
+import cacache from 'cacache';


I would stay with cacache as name. name is usual equal to package name.

sokra · 2017-09-29T14:39:39Z

src/uglify/index.js

+  constructor(options = {}) {
+    const { cache, parallel } = options;
+    this.cacheDir = cache === true ? findCacheDir({ name: 'uglifyjs-webpack-plugin' }) : cache;
+    this.maxConcurrentWorkers = parallel === true ? os.cpus().length - 1 : Math.min(Number(parallel) || 0, os.cpus().length - 1);


Is there a reason why only using cpu count - 1? Why not using all CPUs?

I would replace Math.min(Number(parallel) || 0, os.cpus().length - 1) with Number(parallel) || 0. When the user chooses a worker count we should respect it, even if it's more than CPUs.

@sokra -1 - one for master other for childs

Yep one core needs to be reserved for the main process running the worker farm

sokra · 2017-09-29T14:43:12Z

src/uglify/index.js

-            put(this.cache, task.cacheKey, data, cacheIdentifier).then(done, done);
+
+          if (this.cacheDir && !result.error) {
+            cacache.put(this.cacheDir, task.cacheKey, JSON.stringify(data)).then(done, done);


Does it make sense to move the cache put operation into the worker process?

@sokra no sense

joshwiens · 2017-09-29T14:51:50Z

@evilebottnawi - This in a place where you are ready for a beta build out to npm?

alexander-akait · 2017-09-29T15:28:13Z

@d3viant0ne yep

Genuifx · 2017-10-10T02:31:16Z

why not merge this pr？ some of my project in window system doesn't work now. TT

joshwiens · 2017-10-10T03:22:57Z

@Genuifx - It will merge when we are satisfied with it's stability. In the mean time, there are beta builds available on npm for this branch with the latest being uglifyjs-webpack-plugin@1.0.0-beta.3

Genuifx · 2017-10-10T08:55:10Z

@d3viant0ne copy that.

alexander-akait · 2017-10-10T08:55:55Z

@Genuifx feedback welcome 👍

joshwiens · 2017-10-16T06:51:26Z

@evilebottnawi - Updated defaults & the npm 5 / Node 4.3 issue. One the snapshots are sorted, i'd like to put out a final beta build & get this finished up this week if possible.

genintho · 2017-10-16T19:54:29Z

It will merge when we are satisfied with it's stability

We have been using beta3 at work (Hipmunk) for a few days and have not experienced any issue yet. This represent around 100 build.

alexander-akait added priority: 4 (important) pr_action: Review semver: Patch severity: 4 (inconvenient) type: Refactor labels Aug 8, 2017

alexander-akait requested review from bebraw, hulkish, joshwiens and michael-ciniawsky August 8, 2017 17:13

michael-ciniawsky approved these changes Aug 8, 2017

View reviewed changes

michael-ciniawsky changed the title ~~refactor: respect uglify options and version~~ refactor: respect uglify options and version (options.parallel.cache) Aug 8, 2017

michael-ciniawsky added this to the 1.0.0 milestone Aug 8, 2017

alexander-akait mentioned this pull request Aug 16, 2017

[v1.0.0-beta.2] parallelization isn't working #97

Closed

alexander-akait force-pushed the refactor-and-improve-cache-key branch from 00d75bb to d2ac477 Compare August 16, 2017 20:33

alexander-akait changed the title ~~refactor: respect uglify options and version (options.parallel.cache)~~ refactor: cache and parallel options Aug 16, 2017

alexander-akait force-pushed the refactor-and-improve-cache-key branch 3 times, most recently from e700839 to cc3d49f Compare August 16, 2017 21:01

michael-ciniawsky reviewed Aug 16, 2017

View reviewed changes

michael-ciniawsky added semver: Major and removed semver: Patch labels Aug 16, 2017

alexander-akait mentioned this pull request Aug 17, 2017

[v1.0.0-beta.2] sourcemap not work in parallel #111

Closed

refactor: cache and parallel options

e22ee39

alexander-akait force-pushed the refactor-and-improve-cache-key branch from cc3d49f to e22ee39 Compare August 17, 2017 09:21

alexander-akait removed the request for review from bebraw August 17, 2017 09:25

michael-ciniawsky changed the title ~~refactor: cache and parallel options~~ refactor: split cache and parallel options Sep 12, 2017

michael-ciniawsky approved these changes Sep 12, 2017

View reviewed changes

alexander-akait mentioned this pull request Sep 13, 2017

[v1.0.0-beta.2 (Windows)] Stuck in chunk assets optimization when parallelization is enabled (options.parallel) #118

Closed

sokra approved these changes Sep 29, 2017

View reviewed changes

chore(release): 1.0.0-beta.3

296e95d

alexander-akait mentioned this pull request Oct 5, 2017

Webpack's Performance at Airbnb webpack/webpack#5718

Closed

blowery mentioned this pull request Oct 6, 2017

Build: Remove custom bundler and extract minification Automattic/wp-calypso#18074

Merged

alexander-akait mentioned this pull request Oct 10, 2017

[v1.0.0-beta.3] Broken parallel option #139

Closed

joshwiens added 3 commits October 16, 2017 01:33

chore: Update npm 5 compatible min Node 4 version

b34c088

chore: Updates to defaults 1.6

d2b8a7c

build: Include schema in dist build

5335565

michael-ciniawsky added 2 commits October 21, 2017 04:49

test(ecma): update snapshots

9732a4a

chore(package): uglify-es v3.0.24...3.1.3

c086ef3

michael-ciniawsky added status: Approved and removed priority: 4 (important) pr_action: Review severity: 4 (inconvenient) labels Oct 21, 2017

michael-ciniawsky merged commit d81ef5a into master Oct 21, 2017

michael-ciniawsky deleted the refactor-and-improve-cache-key branch October 21, 2017 03:02

michael-ciniawsky removed this from the 2.0.0 milestone Oct 29, 2017

		@@ -1,10 +1,9 @@
		import os from 'os';
		import cacache from 'cacache';

Uh oh!

refactor: split cache and parallel options #108

refactor: split cache and parallel options #108

Uh oh!

Conversation

alexander-akait commented Aug 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notable Changes

Issues

One strange behavior

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michael-ciniawsky Aug 17, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-ciniawsky commented Aug 16, 2017

Uh oh!

michael-ciniawsky left a comment

Choose a reason for hiding this comment

Uh oh!

alexander-akait commented Aug 17, 2017

Uh oh!

michael-ciniawsky commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshwiens commented Aug 21, 2017

Uh oh!

michael-ciniawsky commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexander-akait commented Sep 12, 2017

Uh oh!

sokra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sokra Sep 29, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshwiens commented Sep 29, 2017

Uh oh!

alexander-akait commented Sep 29, 2017

Uh oh!

Genuifx commented Oct 10, 2017

refactor: split `cache` and `parallel` options #108

refactor: split `cache` and `parallel` options #108

alexander-akait commented Aug 8, 2017 •

edited

Loading

michael-ciniawsky Aug 17, 2017 •

edited

Loading

alexander-akait commented Aug 16, 2017 •

edited

Loading

michael-ciniawsky commented Aug 17, 2017 •

edited

Loading

michael-ciniawsky commented Sep 12, 2017 •

edited

Loading

sokra Sep 29, 2017 •

edited

Loading

joshwiens commented Oct 10, 2017 •

edited

Loading