Create Block: Simplify the process of defining a config for templates #22235
Conversation
gziolo
added
[Type] Enhancement
gziolo
force-pushed
the
update/create-block-glob-templates
branch
from
9a2bf50
to
69da572
Compare
|
Size Change: +3.76 kB (0%) Total Size: 828 kB
ℹ️ View Unchanged
|
gziolo
changed the title
gziolo
force-pushed
the
update/create-block-glob-templates
branch
from
89d83ad
to
f020b6b
Compare
|
@fabiankaegy – I refactored code to make it possible to contain the logic that downloads the npm template in |
gziolo
force-pushed
the
update/create-block-glob-templates
branch
from
a4631c8
to
0565d15
Compare
| '.mustache', | ||
| '' | ||
| ); | ||
| const outputTemplate = await readFile( |
There was a problem hiding this comment.
Probably outside the scope of what you're trying to do here, since as far as I know it's not fundamentally changing anything that doesn't already exist, but: I wonder if we should be lazier here. Rather than eagerly loading every template for every template option, at least only loading the template files for what's relevant given the user input.
It could be a matter of the template string being a function to call when needed, or something more "clever" (not necessarily a good thing) like Object.defineProperty getters, or only getOutputTemplates for templates relevant by input.
The last might also be good to avoid fast-glob if we know we won't need the template files.
There was a problem hiding this comment.
It could be a matter of the template string being a function to call when needed
The last might also be good to avoid fast-glob if we know we won't need the template files.
To clarify, it only loads all templates for a block template selected when calling npm init @wordpress/block. By default, it will only grab files from lib/templates/esnext folder. At the moment, we use all loaded templates to scaffold the project for the block.
Rather than eagerly loading every template for every template option, at least only loading the template files for what's relevant given the user input.
Once we have some conditions that make some templates options, it will make sense to defer loading templates and using a technique like " template string being a function" 👍
Did I miss something?
There was a problem hiding this comment.
To clarify, it only loads all templates for a block template selected when calling
npm init @wordpress/block. By default, it will only grab files fromlib/templates/esnextfolder. At the moment, we use all loaded templates to scaffold the project for the block.
That's good, and largely addresses the concern I had. It was a misinterpretation on my part.
I was thinking about it a little more later in the day yesterday, still misled by the prior concern, but technically still valid: Ideally we try to avoid holding file content in memory longer than necessary. It's something which Node streams excel at, though at a more simple level it could just be a matter of: Only ever having one file loaded in memory at a time, generate the output, move on to the next (rely on garbage collection).
It could have been more of a problem if my previous impression of the implementation was correct, which it's not. Now it may be overkill 😄
There was a problem hiding this comment.
Only ever having one file loaded in memory at a time, generate the output, move on to the next (rely on garbage collection).
We had that before and I liked it more. Here I prematurely assumed that template downloaded from npm would be load into memory and removed instantaneously. Let's see how the final implementation looks like. If we could make it work with npm install my-template --no-save then we could list absolute paths to the individual file instead and that would solve both issues assuming that npm cleans up the node_modules folder when using npm init :)
Co-authored-by: Andrew Duthie <[email protected]>
Description
Related to efforts started by @fabiankaegy in #22175.
The goal here is to make it simplify the process of defining templates:
globdetects them based on the file path that is assumed aslib/templates/${ templateName }for predefined templateswpScriptsare enabled by default, the template author can disable that by setting tofalseThere was general refactoring performed to process the template and create full configuration first and use such an object to pass to all other helper functions. This way, it's going to be way much easier to handle external templates as explored in #22175.
How has this been tested?
npx wp-create-block fun-with-templatenpx wp-create-block fun-with-template -t es5Types of changes
Refactoring.
Checklist: