Skip to content

Build: Change package build step to async flow #8093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aduth merged 3 commits into from
Jan 4, 2019
Merged

Build: Change package build step to async flow #8093

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9feaf4a
Build: Change package build step to async flow
aduth Jan 4, 2019
b6e7084
Build: Trigger LazyResult#then for postcss.process
aduth Jan 4, 2019
a7094d5
Build: Consistently use css property for stringified result
aduth Jan 4, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
107 changes: 62 additions & 45 deletions bin/packages/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,15 @@
/**
* External dependencies
*/
const { promisify } = require( 'util' );
const fs = require( 'fs' );
const path = require( 'path' );
const glob = require( 'glob' );
let glob = require( 'glob' );
const babel = require( '@babel/core' );
const chalk = require( 'chalk' );
const mkdirp = require( 'mkdirp' );
let mkdirp = require( 'mkdirp' );
const sass = require( 'node-sass' );
const postcss = require( 'postcss' );
const deasync = require( 'deasync' );

/**
* Internal dependencies
Expand All @@ -36,6 +36,14 @@ const BUILD_DIR = {
};
const DONE = chalk.reset.inverse.bold.green( ' DONE ' );

// Promisification
const readFile = promisify( fs.readFile );
const writeFile = promisify( fs.writeFile );
const transformFile = promisify( babel.transformFile );
const renderSass = promisify( sass.render );
glob = promisify( glob );
mkdirp = promisify( mkdirp );

/**
* Get the package name for a specified file
*
Expand Down Expand Up @@ -73,6 +81,8 @@ function getBuildPath( file, buildFolder ) {
* Given a list of scss and js filepaths, divide them into sets them and rebuild.
*
* @param {Array} files list of files to rebuild
*
* @return {Promise} Promise resolving when files are built.
*/
function buildFiles( files ) {
// Reduce files into a unique sets of javaScript files and scss packages.
Expand All @@ -87,39 +97,50 @@ function buildFiles( files ) {
return accumulator;
}, { jsFiles: new Set(), scssPackagePaths: new Set() } );

buildPaths.jsFiles.forEach( buildJsFile );
buildPaths.scssPackagePaths.forEach( buildPackageScss );
return Promise.all( [
...buildPaths.jsFiles.map( buildJsFile ),
Copy link
Member Author

@aduth aduth Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ),

...buildPaths.scssPackagePaths.map( buildPackageScss ),
] );
}

/**
* Build a javaScript file for the required environments (node and ES5)
*
* @param {string} file File path to build
* @param {boolean} silent Show logs
*
* @return {Promise} Promise resolving when file is built.
*/
function buildJsFile( file, silent ) {
buildJsFileFor( file, silent, 'main' );
buildJsFileFor( file, silent, 'module' );
return Promise.all( [
buildJsFileFor( file, silent, 'main' ),
buildJsFileFor( file, silent, 'module' ),
] );
}

/**
* Build a package's scss styles
*
* @param {string} packagePath The path to the package.
*
* @return {Promise} Promise resolving when file is built.
*/
function buildPackageScss( packagePath ) {
async function buildPackageScss( packagePath ) {
const srcDir = path.resolve( packagePath, SRC_DIR );
const scssFiles = glob.sync( `${ srcDir }/*.scss` );
const scssFiles = await glob( `${ srcDir }/*.scss` );

// Build scss files individually.
scssFiles.forEach( buildScssFile );
return Promise.all( scssFiles.map( buildScssFile ) );
}

function buildScssFile( styleFile ) {
async function buildScssFile( styleFile ) {
const outputFile = getBuildPath( styleFile.replace( '.scss', '.css' ), BUILD_DIR.style );
const outputFileRTL = getBuildPath( styleFile.replace( '.scss', '-rtl.css' ), BUILD_DIR.style );
mkdirp.sync( path.dirname( outputFile ) );
const builtSass = sass.renderSync( {

await mkdirp( path.dirname( outputFile ) );

const contents = await readFile( styleFile, 'utf8' );
const builtSass = await renderSass( {
file: styleFile,
includePaths: [ path.resolve( __dirname, '../../assets/stylesheets' ) ],
data: (
Expand All @@ -131,27 +152,22 @@ function buildScssFile( styleFile ) {
'animations',
'z-index',
].map( ( imported ) => `@import "${ imported }";` ).join( ' ' ) +
fs.readFileSync( styleFile, 'utf8' )
contents
),
} );

const postCSSSync = ( callback ) => {
postcss( require( './post-css-config' ) )
.process( builtSass.css, { from: 'src/app.css', to: 'dest/app.css' } )
.then( ( result ) => callback( null, result ) );
};

const postCSSRTLSync = ( ltrCSS, callback ) => {
postcss( [ require( 'rtlcss' )() ] )
.process( ltrCSS, { from: 'src/app.css', to: 'dest/app.css' } )
.then( ( result ) => callback( null, result ) );
};
const result = await postcss( require( './post-css-config' ) ).process( builtSass.css, {
from: 'src/app.css',
to: 'dest/app.css',
} );

const result = deasync( postCSSSync )();
fs.writeFileSync( outputFile, result.css );
const resultRTL = await postcss( [ require( 'rtlcss' )() ] ).process( result.css, {
from: 'src/app.css',
to: 'dest/app.css',
} );

const resultRTL = deasync( postCSSRTLSync )( result );
fs.writeFileSync( outputFileRTL, resultRTL );
await writeFile( outputFile, result.css );
await writeFile( outputFileRTL, resultRTL.css );
}

/**
Expand All @@ -161,17 +177,17 @@ function buildScssFile( styleFile ) {
* @param {boolean} silent Show logs
* @param {string} environment Dist environment (node or es5)
*/
function buildJsFileFor( file, silent, environment ) {
async function buildJsFileFor( file, silent, environment ) {
const buildDir = BUILD_DIR[ environment ];
const destPath = getBuildPath( file, buildDir );
const babelOptions = getBabelConfig( environment );
babelOptions.sourceMaps = true;
babelOptions.sourceFileName = file;

mkdirp.sync( path.dirname( destPath ) );
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 ) );
Copy link
Member Author

@aduth aduth Jan 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const transformed = await transformFile( file, babelOptions );
writeFile( destPath + '.map', JSON.stringify( transformed.map ) );
Copy link
Member Author

@aduth aduth Jan 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

writeFile( destPath, transformed.code + '\n//# sourceMappingURL=' + path.basename( destPath ) + '.map' );

if ( ! silent ) {
process.stdout.write(
Expand All @@ -189,24 +205,25 @@ function buildJsFileFor( file, silent, environment ) {
*
* @param {string} packagePath absolute package path
*/
function buildPackage( packagePath ) {
async function buildPackage( packagePath ) {
const srcDir = path.resolve( packagePath, SRC_DIR );
const jsFiles = glob.sync( `${ srcDir }/**/*.js`, {
const jsFiles = await glob( `${ srcDir }/**/*.js`, {
ignore: [
`${ srcDir }/**/test/**/*.js`,
`${ srcDir }/**/__mocks__/**/*.js`,
],
nodir: true,
} );

process.stdout.write( `${ path.basename( packagePath ) }\n` );

// Build js files individually.
jsFiles.forEach( ( file ) => buildJsFile( file, true ) );
await Promise.all( [
// Build js files individually.
...jsFiles.map( ( file ) => buildJsFile( file, true ) ),

// Build package CSS files
buildPackageScss( packagePath );
// Build package CSS files
buildPackageScss( packagePath ),
] );

process.stdout.write( `${ path.basename( packagePath ) }\n` );
process.stdout.write( `${ DONE }\n` );
}

Expand All @@ -216,7 +233,7 @@ if ( files.length ) {
buildFiles( files );
} else {
process.stdout.write( chalk.inverse( '>> Building packages \n' ) );
getPackages()
.forEach( buildPackage );
process.stdout.write( '\n' );
Promise.all( getPackages().map( buildPackage ) ).then( () => {
process.stdout.write( '\n' );
} );
}
16 changes: 0 additions & 16 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
"cross-env": "3.2.4",
"cssnano": "4.0.3",
"enzyme": "3.7.0",
"deasync": "0.1.13",
Copy link
Member

@gziolo gziolo Jul 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice 👍

"deep-freeze": "0.0.1",
"doctrine": "2.1.0",
"eslint-plugin-jest": "21.5.0",
Expand Down