Try to generalize build transforms #16317
Conversation
talldan
added
[Type] Task
talldan
requested review from
aduth,
ajitbohra,
gziolo,
nerrad,
oandregal,
ntwb and
youknowriad
as code owners
| const packages = new Set; | ||
| function createEntryTransform( mappingFunc ) { | ||
| // Track any already built files to avoid duplication. | ||
| const fileSet = new Set; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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).
talldan
changed the title
| // Only operate once per package, assuming entries are common. | ||
| const packageName = getPackageName( file ); | ||
| if ( packages.has( packageName ) ) { | ||
| if ( isString( mapped ) && ! fileSet.has( mapped ) ) { |
There was a problem hiding this comment.
Could we just normalize this value to an array and skip directly to the forEach below?
|
|
||
| // Build all root level stylesheets for the package. | ||
| const packageName = getPackageName( file ); | ||
| const rootScssFiles = await glob( path.resolve( PACKAGES_DIR, packageName, 'src/*.scss' ) ); |
There was a problem hiding this comment.
The capitalization of this as an acronym should be rootSCSSFiles.
That said, I don't think we need a variable here, just return await ...
| const packages = new Set; | ||
| function createEntryTransform( mappingFunc ) { | ||
| // Track any already built files to avoid duplication. | ||
| const fileSet = new Set; |
There was a problem hiding this comment.
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).
|
Closing this for now—happy for it to be picked up by someone else, but not seeing much value on working on it more. |
Description
As mentioned in #16150 (comment), the solution for handling block.json files when
npm run devis running introduces some code duplication. This PR tries to reduce that duplicationHow has this been tested?
Test that
npm run buildandnpm run devcontinue to work.Types of changes
Refactor (non-breaking change which adds functionality)
Checklist: