Build: Change package build step to async flow #8093

aduth · 2018-07-20T18:10:55Z

This pull request seeks to adapt the package build script to perform builds in parallel, rather than in sequence. In practice, this unfortunately does not appear to make a significant difference, reducing the build time for me from about 12s to 10s. It seems that the style build takes a significant part of the time of package build.

Noting also that this is not a total conversion. getPackages should be made to be asynchronous, but is touched by other scripts as well. As such, it was decided to be left as a separate task.

Testing instructions:

There should be no regressions in the build of packages.

youknowriad · 2018-07-25T09:21:28Z

bin/packages/build.js

+	return Promise.all( [
+		...files.map( ( file ) => buildFile( file, true ) ),
+		buildPackageStyleIfApplicable(),
+	] ).then( () => {


This could be replaced with await ?

This could be replaced with await ?

Yep, updated in rebased 9feaf4a.

youknowriad

LGTM 👍

gziolo · 2018-07-23T06:29:10Z

bin/packages/build.js

-		.forEach( buildPackage );
-	process.stdout.write( '\n' );
+
+	Promise.all( [


Shouldn't it be:

Promise.all( getPackages().map( buildPackage ) )...

?

Shouldn't it be:

Promise.all( getPackages().map( buildPackage ) )...

?

Updated in rebased 9feaf4a.

gziolo · 2018-07-23T06:32:09Z

package.json

@@ -89,7 +89,6 @@
 		"concurrently": "3.5.0",
 		"core-js": "2.5.7",
 		"cross-env": "3.2.4",
-		"deasync": "0.1.13",


gziolo · 2018-08-01T09:05:28Z

This one needs rebase and some adjustments after changes introduced in #8187 for RTL support for CSS.

aduth · 2019-01-04T14:45:04Z

I've finally brought this back up to date. Unfortunately it still doesn't seem to have much of an overall impact.

gziolo · 2019-01-04T14:50:28Z

Feel free to merge when Travis is happy. It was already reviewed :)

I've finally brought this back up to date. Unfortunately it still doesn't seem to have much of an overall impact.

@noisysocks has PR opened which adds caching, maybe it will have an impact together.

aduth · 2019-01-04T15:38:28Z

I have two of my own remaining reservations:

I'm not sure about postcss().process being synchronous as it was refactored here. The function is a bit confusing, where it returns a "LazyResult" object which has both an evaluated css property but also is then-able. By the documentation of then, I'm inclined to think that it must be used if needing to account for asynchronous postcss plugins.
I'm not sure about unbounded concurrency for the build. It's probably not much of an issue to build our current set of modules in parallel, but it might be worth considering something like p-limit to batch a set maximum number of builds to occur at a time.

aduth · 2019-01-04T15:40:39Z

Ah, I guess I should have read those links more closely, as it answers my question quite concretely 😅

This is why this method is only for debug purpose, you should always use LazyResult#then.

http://api.postcss.org/LazyResult.html#css

See: http://api.postcss.org/LazyResult.html#css >This is why this method is only for debug purpose, you should always use LazyResult#then.

Effectively unchanged, since stringification of the LazyResult object is aliased to the css property. See: http://api.postcss.org/LazyResult.html#toString

aduth · 2019-01-04T16:49:03Z

LazyResult is now used as a thenable as of b6e7084.

I'm fine in the initial approach to have unbounded parallelization. This can be revisited if it's shown to be problematic.

This reverts commit 4928f16.

aduth · 2019-01-04T18:32:51Z

There's a few issues here impacting (a) sourcemaps, (b) npm run dev regeneration, and (c) a possible race condition with writeFile.

I've proposed a revert at #13195

I have an in-progress branch with some partial fixes at fix/mkdirp-source-maps.

I'll leave a few follow-up comments here for more specific problems.

aduth · 2019-01-04T18:34:34Z

bin/packages/build.js

-	buildPaths.jsFiles.forEach( buildJsFile );
-	buildPaths.scssPackagePaths.forEach( buildPackageScss );
+	return Promise.all( [
+		...buildPaths.jsFiles.map( buildJsFile ),


I was wrong in thinking that by the previous implementation using forEach, these were arrays, when in fact they're of type Set. Thus, rebuilds fail with an error:

[1] /Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/bin/packages/build.js:100 [1] ...buildPaths.jsFiles.map( buildJsFile ), [1] ^ [1] [1] TypeError: buildPaths.jsFiles.map(...) is not a function or its return value is not iterable [1] at buildFiles (/Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/bin/packages/build.js:100:25) [1] at Object.<anonymous> (/Users/andrew/Documents/Code/vvv/www/editor/htdocs/wp-content/plugins/gutenberg/bin/packages/build.js:234:2) [1] at Module._compile (internal/modules/cjs/loader.js:689:30) [1] at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10) [1] at Module.load (internal/modules/cjs/loader.js:599:32) [1] at tryModuleLoad (internal/modules/cjs/loader.js:538:12) [1] at Function.Module._load (internal/modules/cjs/loader.js:530:3) [1] at Function.Module.runMain (internal/modules/cjs/loader.js:742:12) [1] at startup (internal/bootstrap/node.js:282:19) [1] at bootstrapNodeJSCore (internal/bootstrap/node.js:743:3)

Two alternatives are:

Don't track them as a Set, rather as an array and guarantee uniqueness before pushing to array.

Convert Set to array at this line before the map, i.e. ...[ ...buildPaths.jsFiles ].map( buildJsFile ),

aduth · 2019-01-04T18:35:24Z

bin/packages/build.js

-	fs.writeFileSync( destPath, transformed.code + '\n//# sourceMappingURL=' + path.basename( destPath ) + '.map' );
+	await mkdirp( path.dirname( destPath ) );
+	const transformed = await transformFile( file, babelOptions );
+	writeFile( destPath + '.map', JSON.stringify( transformed.map ) );


These are asynchronous tasks and should have await, preferably in parallel as await Promise.all between writing the map file and the original source.

aduth · 2019-01-04T18:36:54Z

bin/packages/build.js

-	const transformed = babel.transformFileSync( file, babelOptions );
-	fs.writeFileSync( destPath + '.map', JSON.stringify( transformed.map ) );
-	fs.writeFileSync( destPath, transformed.code + '\n//# sourceMappingURL=' + path.basename( destPath ) + '.map' );
+	await mkdirp( path.dirname( destPath ) );


I'm seeing varied results in attributing this to be a cause, but I was seeing some improvement by switching this back to its synchronous form.

Possibly related: https://github.com/substack/node-mkdirp/issues/154

Viable substitute module: https://www.npmjs.com/package/make-dir

talldan · 2019-01-07T02:42:27Z

I had a similar attempt at this over christmas (didn't realise this PR existed). I also didn't see any real improvement in build times, so didn't bother tidying things up:
#13104

It'll be worth keeping an eye on node 10's promise based filesystem apis. They're currently marked as experimental, hopefully in a not too distant future version they'll become stable:
https://nodejs.org/docs/latest-v10.x/api/fs.html#fs_fs_promises_api

This reverts commit 4928f16.

* Build: Change package build step to async flow * Build: Trigger LazyResult#then for postcss.process See: http://api.postcss.org/LazyResult.html#css >This is why this method is only for debug purpose, you should always use LazyResult#then. * Build: Consistently use css property for stringified result Effectively unchanged, since stringification of the LazyResult object is aliased to the css property. See: http://api.postcss.org/LazyResult.html#toString

This reverts commit 4928f16.

* Build: Change package build step to async flow * Build: Trigger LazyResult#then for postcss.process See: http://api.postcss.org/LazyResult.html#css >This is why this method is only for debug purpose, you should always use LazyResult#then. * Build: Consistently use css property for stringified result Effectively unchanged, since stringification of the LazyResult object is aliased to the css property. See: http://api.postcss.org/LazyResult.html#toString

This reverts commit 4928f16.

aduth added [Type] Build Tooling Issues or PRs related to build tooling npm Packages Related to npm packages labels Jul 20, 2018

aduth requested a review from youknowriad July 20, 2018 18:10

youknowriad reviewed Jul 25, 2018

View reviewed changes

youknowriad approved these changes Jul 25, 2018

View reviewed changes

gziolo reviewed Jul 25, 2018

View reviewed changes

gziolo added this to the 3.4 milestone Jul 25, 2018

gziolo mentioned this pull request Jul 25, 2018

Packages: Adding RTL CSS support to the packages CSS #8187

Merged

pento modified the milestones: 3.4, 3.5 Jul 30, 2018

aduth removed this from the 3.5 milestone Aug 7, 2018

gziolo added this to the 3.8 milestone Aug 30, 2018

youknowriad modified the milestones: 3.8, 3.9 Sep 5, 2018

youknowriad removed this from the 3.9 milestone Sep 13, 2018

Build: Change package build step to async flow

9feaf4a

aduth force-pushed the update/async-package-build branch from c9b776b to 9feaf4a Compare January 4, 2019 14:44

aduth added 2 commits January 4, 2019 10:51

Build: Trigger LazyResult#then for postcss.process

b6e7084

See: http://api.postcss.org/LazyResult.html#css >This is why this method is only for debug purpose, you should always use LazyResult#then.

Build: Consistently use css property for stringified result

a7094d5

Effectively unchanged, since stringification of the LazyResult object is aliased to the css property. See: http://api.postcss.org/LazyResult.html#toString

aduth merged commit 4928f16 into master Jan 4, 2019

aduth deleted the update/async-package-build branch January 4, 2019 16:49

aduth added a commit that referenced this pull request Jan 4, 2019

Revert "Build: Change package build step to async flow (#8093)"

12fd0eb

This reverts commit 4928f16.

aduth mentioned this pull request Jan 4, 2019

Revert "Build: Change package build step to async flow" #13195

Merged

aduth commented Jan 4, 2019

View reviewed changes

aduth added a commit that referenced this pull request Jan 7, 2019

Revert "Build: Change package build step to async flow (#8093)" (#13195)

68f135b

This reverts commit 4928f16.

youknowriad added this to the 4.9 (Gutenberg) milestone Jan 18, 2019

gziolo mentioned this pull request Jan 21, 2019

[WIP] Try caching compiled JavaScript when building packages #13124

Closed

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Revert "Build: Change package build step to async flow (#8093)" (#13195)

5203deb

This reverts commit 4928f16.

youknowriad pushed a commit that referenced this pull request Mar 6, 2019

Revert "Build: Change package build step to async flow (#8093)" (#13195)

0dfaf87

This reverts commit 4928f16.

aduth mentioned this pull request Apr 26, 2019

Build Tooling: Optimize build by spawning worker pool #15230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build: Change package build step to async flow #8093

Build: Change package build step to async flow #8093

aduth commented Jul 20, 2018

youknowriad Jul 25, 2018

aduth Jan 4, 2019

youknowriad left a comment

gziolo Jul 23, 2018

aduth Jan 4, 2019

gziolo Jul 23, 2018

gziolo commented Aug 1, 2018

aduth commented Jan 4, 2019

gziolo commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth Jan 4, 2019 •

edited

Loading

aduth Jan 4, 2019

aduth Jan 4, 2019 •

edited

Loading

talldan commented Jan 7, 2019

Build: Change package build step to async flow #8093

Build: Change package build step to async flow #8093

Conversation

aduth commented Jul 20, 2018

youknowriad Jul 25, 2018

Choose a reason for hiding this comment

aduth Jan 4, 2019

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

gziolo Jul 23, 2018

Choose a reason for hiding this comment

aduth Jan 4, 2019

Choose a reason for hiding this comment

gziolo Jul 23, 2018

Choose a reason for hiding this comment

gziolo commented Aug 1, 2018

aduth commented Jan 4, 2019

gziolo commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth commented Jan 4, 2019

aduth Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

aduth Jan 4, 2019

Choose a reason for hiding this comment

aduth Jan 4, 2019 • edited Loading

Choose a reason for hiding this comment

talldan commented Jan 7, 2019

aduth Jan 4, 2019 •

edited

Loading

aduth Jan 4, 2019 •

edited

Loading