Skip to content
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

Try to generalize build transforms #16317

Closed
wants to merge 2 commits into from
Closed

Try to generalize build transforms #16317

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e38443
Reduce duplication of code for stream transforms
talldan Jun 26, 2019
6cb1953
Avoid duplicate file builds in a more generic fashion
talldan Jun 26, 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
100 changes: 60 additions & 40 deletions bin/packages/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/**
* External dependencies
*/
const { isString } = require( 'lodash' );
const path = require( 'path' );
const glob = require( 'fast-glob' );
const ProgressBar = require( 'progress' );
Expand All @@ -29,41 +30,77 @@ function getPackageName( file ) {
}

/**
* Returns a stream transform which maps an individual stylesheet to its
* package entrypoint. Unlike JavaScript which uses an external bundler to
* efficiently manage rebuilds by entrypoints, stylesheets are rebuilt fresh
* in their entirety from the build script.
* Helper higher order function for creating stream transforms that map from
* one input file to none, one, or multiple output files.
*
* @param {Function} mappingFunc An async mapping function that receives a file
* path and returns either a falsy to indicate
* the file should not be built, a string file
* path to indicate that this single file should
* be built, or an array of strings to indicate
* multiple files should be built.
*
* @return {Transform} Stream transform instance.
*/
function createStyleEntryTransform() {
const packages = new Set;
function createEntryTransform( mappingFunc ) {
// Track any already built files to avoid duplication.
const fileSet = new Set;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second commit in this branch introduces this generic way to avoid duplicate built files, but I'm not sure it's the right solution.

It doesn't avoid repeating expensive operations in the mappingFunc, which I imagine was the original intention for tracking changed packages. (cc @aduth)

Copy link
Member

Choose a reason for hiding this comment

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

Hm, yeah, I see what you mean. Maybe we can pass fileSet as the second argument in the mapping function to allow the callback to determine whether to defer to a cached value?

Or if we really want to follow Array#map signature, as the third argument, and likely as an array and not a set (documentation).


return new Transform( {
objectMode: true,
async transform( file, encoding, callback ) {
// Only stylesheets are subject to this transform.
if ( path.extname( file ) !== '.scss' ) {
this.push( file );
const mapped = await mappingFunc( file );

// Ignore this file when a falsey is provided.
if ( ! mapped ) {
callback();
return;
}

// Only operate once per package, assuming entries are common.
const packageName = getPackageName( file );
if ( packages.has( packageName ) ) {
if ( isString( mapped ) && ! fileSet.has( mapped ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could we just normalize this value to an array and skip directly to the forEach below?

https://lodash.com/docs/4.17.11#castArray

// Handle a single path.
this.push( mapped );
fileSet.add( mapped );
callback();
return;
}

packages.add( packageName );
const entries = await glob( path.resolve( PACKAGES_DIR, packageName, 'src/*.scss' ) );
entries.forEach( ( entry ) => this.push( entry ) );
callback();
if ( Array.isArray( mapped ) ) {
// Handle an array of mapped paths.
mapped.forEach( ( filePath ) => {
if ( ! fileSet.has( filePath ) ) {
this.push( filePath );
fileSet.add( mapped );
}
} );
callback();
}
},
} );
}

/**
* Returns a stream transform which maps an individual stylesheet to its
* package entrypoint. Unlike JavaScript which uses an external bundler to
* efficiently manage rebuilds by entrypoints, stylesheets are rebuilt fresh
* in their entirety from the build script.
*
* @return {Transform} Stream transform instance.
*/
function createStyleEntryTransform() {
return createEntryTransform( async ( file ) => {
// Only stylesheets are subject to this transform.
if ( path.extname( file ) !== '.scss' ) {
return file;
}

// Build all root level stylesheets for the package.
const packageName = getPackageName( file );
const rootScssFiles = await glob( path.resolve( PACKAGES_DIR, packageName, 'src/*.scss' ) );
Copy link
Member

Choose a reason for hiding this comment

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

The capitalization of this as an acronym should be rootSCSSFiles.

https://make.wordpress.org/core/handbook/best-practices/coding-standards/javascript/#abbreviations-and-acronyms

That said, I don't think we need a variable here, just return await ...

return rootScssFiles;
} );
}

/**
* Returns a stream transform which maps an individual block.json to the
* index.js that imports it. Presently, babel resolves the import of json
Expand All @@ -73,31 +110,14 @@ function createStyleEntryTransform() {
* @return {Transform} Stream transform instance.
*/
function createBlockJsonEntryTransform() {
const blocks = new Set;

return new Transform( {
objectMode: true,
async transform( file, encoding, callback ) {
const matches = /block-library[\/\\]src[\/\\](.*)[\/\\]block.json$/.exec( file );
const blockName = matches ? matches[ 1 ] : undefined;

// Only block.json files in the block-library folder are subject to this transform.
if ( ! blockName ) {
this.push( file );
callback();
return;
}

// Only operate once per block, assuming entries are common.
if ( blockName && blocks.has( blockName ) ) {
callback();
return;
}
return createEntryTransform( async ( file ) => {
// Only block.json files in the block-library folder are subject to this transform.
if ( ! /block-library[\/\\]src[\/\\].*[\/\\]block.json$/.test( file ) ) {
return file;
}

blocks.add( blockName );
this.push( file.replace( 'block.json', 'index.js' ) );
callback();
},
// Build the block's index.js.
return file.replace( 'block.json', 'index.js' );
} );
}

Expand Down