Skip to content

enhancement/issue 823 ensure greenwood cli and plugins are favoring idiomatic usages of async #1546

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
thescientist13 merged 18 commits into from
Aug 23, 2025
Merged

enhancement/issue 823 ensure greenwood cli and plugins are favoring idiomatic usages of async #1546

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
a805a36
chore: refactor async callbacks
KaiPrince Aug 3, 2025
5aa3439
chore: remove promise.resolve/reject in async functions
KaiPrince Aug 4, 2025
2a8fcf8
Revert "chore: remove promise.resolve/reject in async functions"
KaiPrince Aug 4, 2025
1233214
refactor one function
KaiPrince Aug 7, 2025
f3575b8
use clone consistently
KaiPrince Aug 11, 2025
b730cbd
Revert "refactor one function"
KaiPrince Aug 11, 2025
4ecc2bf
Update asyncMap
KaiPrince Aug 11, 2025
5674288
use asyncMap
KaiPrince Aug 11, 2025
b65ee02
use asyncForEach
KaiPrince Aug 16, 2025
6fcbf11
Merge remote-tracking branch 'upstream/master' into enhancement/issue…
KaiPrince Aug 19, 2025
41e0acc
Add eslint await in loop rule
KaiPrince Aug 19, 2025
7e3f0c8
fix: revert merged response .clone cleanup
KaiPrince Aug 21, 2025
a5af458
use `return await` for better stack trace
KaiPrince Aug 21, 2025
2159832
await forEach
KaiPrince Aug 21, 2025
80afa74
await more forEach
KaiPrince Aug 21, 2025
2a591b0
revert no await in loop eslint rule
KaiPrince Aug 21, 2025
7f2339a
Merge branch 'master' into enhancement/issue-823-ensure-Greenwood-CLI…
KaiPrince Aug 21, 2025
6da1e3b
add return await for better stack trace
KaiPrince Aug 21, 2025
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
8 changes: 1 addition & 7 deletions packages/cli/src/commands/build.js
View file
Open in desktop

@KaiPrince KaiPrince Aug 4, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I recommend hiding whitespace changes for easier reviewing

Original file line number Diff line number Diff line change
Expand Up @@ -38,13 +38,7 @@ const runProductionBuild = async (compilation) => {
});
}

await Promise.all(
servers.map(async (server) => {
await server.start();

return Promise.resolve(server);
}),
);
await Promise.all(servers.map((server) => server.start()));

@thescientist13 thescientist13 Aug 5, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nice, this looks very elegant! 🤓


if (prerenderPlugin.executeModuleUrl) {
await preRenderCompilationWorker(compilation, prerenderPlugin);
Expand Down
18 changes: 12 additions & 6 deletions packages/cli/src/lib/async-utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,27 +1,33 @@
// https://stackoverflow.com/a/76974728/417806
// Constraint: callback functions must not depend on each other
async function asyncFilter(arr, cb) {
const filtered = [];

for (const element of arr) {
await asyncForEach(arr, async (element) => {
const needAdd = await cb(element);

if (needAdd) {
filtered.push(element);
}
}
});

return filtered;
}

// https://stackoverflow.com/a/71278238/417806
// Constraint: mapper functions must not depend on each other

@KaiPrince KaiPrince Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Documented a constraint here, since the callbacks need to be safe to run concurrently.

async function asyncMap(items, mapper) {
const mappedItems = [];
const promises = [];

for (const item of items) {
mappedItems.push(await mapper(item));
promises.push(mapper(item));
Comment on lines -21 to +23

@KaiPrince KaiPrince Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Using Promise.all rather than awaiting in the for loop allows them to run in parallel. This technique also matches the stack overflow source linked in the comment.

}

return mappedItems;
return await Promise.all(promises);
}

async function asyncForEach(items, callback) {

@thescientist13 thescientist13 Aug 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I think I must have missed this in a previous review but why would we need a custom asyncForEach? Wouldn't a for..or and / or for ... in achieve the same result, without needing a custom utility?

@KaiPrince KaiPrince Aug 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

the difference is that this wraps them in Promise.all, which allows each callback to run concurrently for better performance.

Often, the code can be refactored to create all the promises at once, then get access to the results using Promise.all() (or one of the other promise concurrency methods). Otherwise, each successive operation will not start until the previous one has completed.

https://eslint.org/docs/latest/rules/no-await-in-loop

This video is a good recap of async/await: https://youtu.be/vn3tm0quoqE?t=341. I recommend you watch it to make sure we're on the same page.

@thescientist13 thescientist13 Aug 21, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ah yes, I think you may have mentioned that before and it just passed me by. 🤦‍♂️

So basically in cases where we want to run a bunch of independent tasks that don't depend on each other, we can run them concurrently using your new asyncForEach function. As opposed to a situation in the case of running some Greenwood plugins over some code / files to transform them in order, that's where using a for ... of / for ... in would be more appropriate.

Good stuff, thanks for taking the time to explain your approach here, much appreciated! 🙌

@thescientist13 thescientist13 Aug 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That video link was great btw, Fireship is the best! 🔥

await asyncMap(items, callback);
}

export { asyncFilter, asyncMap };
export { asyncFilter, asyncMap, asyncForEach };
167 changes: 68 additions & 99 deletions packages/cli/src/lifecycles/bundle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
import path from "node:path";
import { rollup } from "rollup";
import { pruneGraph } from "../lib/content-utils.js";
import { asyncForEach } from "../lib/async-utils.js";

async function interceptPage(url, request, plugins, body) {
let response = new Response(body, {
Expand Down Expand Up @@ -101,65 +102,61 @@ async function emitResources(compilation) {
async function cleanUpResources(compilation) {
const { outputDir } = compilation.context;

for (const resource of compilation.resources.values()) {
await asyncForEach(compilation.resources.values(), async (resource) => {

@thescientist13 thescientist13 Aug 21, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

yeah, for example, why create our custom utility here when we already have the language construct built-in? I think anywhere where we were actually using for...of / for...in was fine as is, that's the end state we wanted the whole time, but obviously map and filter have special behaviors, hence the custom utilities.

@KaiPrince KaiPrince Aug 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Building on my explanation in the other comment, this is a case where we want to take advantage of parallelism. As an example, assume we need to delete ("unlink") 100 files.

In the original implementation, each fs.unlink call doesn't start until the previous one finishes. i.e. the first runs, then the second, and so on. The call to delete the 100th file is only started after the other 99 files are deleted.

In the new implementation, each iteration is wrapped in a Promise.all, which means they run concurrently. All 100 fs.unlink calls are started, and then we wait for them all to finish before returning from this function. Since the filesystem calls are performed by the OS, they run outside of the main JS thread and are actually run in parallel. This results in performance improvements since we get to take advantage of the OS using multiple threads.

const { src, optimizedFileName, optimizationAttr } = resource;
const optConfig = ["inline", "static"].indexOf(compilation.config.optimization) >= 0;
const optAttr = ["inline", "static"].indexOf(optimizationAttr) >= 0;

if (optimizedFileName && (!src || optAttr || optConfig)) {
await fs.unlink(new URL(`./${optimizedFileName}`, outputDir));
}
}
});
}

async function optimizeStaticPages(compilation, plugins) {
const { scratchDir, outputDir } = compilation.context;

return Promise.all(
compilation.graph
.filter(
(page) =>
!page.isSSR ||
(page.isSSR && page.prerender) ||
(page.isSSR && compilation.config.prerender),
)
.map(async (page) => {
const { route, outputHref } = page;
const outputDirUrl = new URL(outputHref.replace("index.html", "").replace("404.html", ""));
const url = new URL(`http://localhost:${compilation.config.port}${route}`);
const contents = await fs.readFile(
new URL(`./${outputHref.replace(outputDir.href, "")}`, scratchDir),
"utf-8",
);
const headers = new Headers({ "Content-Type": "text/html" });
let response = new Response(contents, { headers });

if (!(await checkResourceExists(outputDirUrl))) {
await fs.mkdir(outputDirUrl, {
recursive: true,
});
}
const pages = compilation.graph.filter(
(page) =>
!page.isSSR || (page.isSSR && page.prerender) || (page.isSSR && compilation.config.prerender),

@KaiPrince KaiPrince Aug 11, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There's an unintentional whitespace change here, but the linter requires this change.

);

for (const plugin of plugins) {
if (plugin.shouldOptimize && (await plugin.shouldOptimize(url, response.clone()))) {
const currentResponse = await plugin.optimize(url, response.clone());
await asyncForEach(pages, async (page) => {
const { route, outputHref } = page;
const outputDirUrl = new URL(outputHref.replace("index.html", "").replace("404.html", ""));
const url = new URL(`http://localhost:${compilation.config.port}${route}`);
const contents = await fs.readFile(
new URL(`./${outputHref.replace(outputDir.href, "")}`, scratchDir),
"utf-8",
);
const headers = new Headers({ "Content-Type": "text/html" });
let response = new Response(contents, { headers });

if (!(await checkResourceExists(outputDirUrl))) {
await fs.mkdir(outputDirUrl, {
recursive: true,
});
}

response = mergeResponse(response.clone(), currentResponse.clone());
}
}
for (const plugin of plugins) {
if (plugin.shouldOptimize && (await plugin.shouldOptimize(url, response.clone()))) {
const currentResponse = await plugin.optimize(url, response.clone());

// clean up optimization markers
const body = (await response.text()).replace(/data-gwd-opt=".*?[a-z]"/g, "");
response = mergeResponse(response.clone(), currentResponse.clone());
}
}

await fs.writeFile(new URL(outputHref), body);
}),
);
// clean up optimization markers
const body = (await response.text()).replace(/data-gwd-opt=".*?[a-z]"/g, "");

await fs.writeFile(new URL(outputHref), body);
});
}

async function bundleStyleResources(compilation, resourcePlugins) {
const { outputDir } = compilation.context;

for (const resource of compilation.resources.values()) {
await asyncForEach(compilation.resources.values(), async (resource) => {
const { contents, src = "", type } = resource;

if (["style", "link"].includes(type)) {
Expand Down Expand Up @@ -202,84 +199,58 @@ async function bundleStyleResources(compilation, resourcePlugins) {
const request = new Request(url, { headers });
const initResponse = new Response(contents, { headers });

let response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
const intermediateResponse = await responsePromise;
let response = initResponse;

for (const plugin of resourcePlugins) {
Comment thread
KaiPrince marked this conversation as resolved.
const shouldServe = plugin.shouldServe && (await plugin.shouldServe(url, request));

if (shouldServe) {
const currentResponse = await plugin.serve(url, request);
const mergedResponse = mergeResponse(
intermediateResponse.clone(),
currentResponse.clone(),
);
const mergedResponse = mergeResponse(response.clone(), currentResponse.clone());

if (mergedResponse.headers.get("Content-Type").indexOf(contentType) >= 0) {
return Promise.resolve(mergedResponse.clone());
response = mergedResponse.clone();
}
}
}

return Promise.resolve(responsePromise);
}, Promise.resolve(initResponse));

response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
const intermediateResponse = await responsePromise;
for (const plugin of resourcePlugins) {
const shouldPreIntercept =
plugin.shouldPreIntercept &&
(await plugin.shouldPreIntercept(url, request, intermediateResponse.clone()));
(await plugin.shouldPreIntercept(url, request, response.clone()));

if (shouldPreIntercept) {
const currentResponse = await plugin.preIntercept(
url,
request,
intermediateResponse.clone(),
);
const mergedResponse = mergeResponse(
intermediateResponse.clone(),
currentResponse.clone(),
);
const currentResponse = await plugin.preIntercept(url, request, response.clone());
const mergedResponse = mergeResponse(response.clone(), currentResponse.clone());

if (mergedResponse.headers.get("Content-Type").indexOf(contentType) >= 0) {
return Promise.resolve(mergedResponse.clone());
response = mergedResponse.clone();
}
}
}

return Promise.resolve(responsePromise);
}, Promise.resolve(response.clone()));

response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
const intermediateResponse = await responsePromise;
for (const plugin of resourcePlugins) {
const shouldIntercept =
plugin.shouldIntercept &&
(await plugin.shouldIntercept(url, request, intermediateResponse.clone()));
plugin.shouldIntercept && (await plugin.shouldIntercept(url, request, response.clone()));

if (shouldIntercept) {
const currentResponse = await plugin.intercept(
url,
request,
intermediateResponse.clone(),
);
const mergedResponse = mergeResponse(
intermediateResponse.clone(),
currentResponse.clone(),
);
const currentResponse = await plugin.intercept(url, request, response.clone());
const mergedResponse = mergeResponse(response.clone(), currentResponse.clone());

if (mergedResponse.headers.get("Content-Type").indexOf(contentType) >= 0) {
return Promise.resolve(mergedResponse.clone());
response = mergedResponse.clone();
}
}
}

return Promise.resolve(responsePromise);
}, Promise.resolve(response.clone()));

response = await resourcePlugins.reduce(async (responsePromise, plugin) => {
const intermediateResponse = await responsePromise;
for (const plugin of resourcePlugins) {
const shouldOptimize =
plugin.shouldOptimize && (await plugin.shouldOptimize(url, intermediateResponse.clone()));
plugin.shouldOptimize && (await plugin.shouldOptimize(url, response.clone()));

return shouldOptimize
? Promise.resolve(await plugin.optimize(url, intermediateResponse.clone()))
: Promise.resolve(responsePromise);
}, Promise.resolve(response.clone()));
if (shouldOptimize) {
response = await plugin.optimize(url, response.clone());
}
}

optimizedFileContents = await response.text();

Expand All @@ -291,7 +262,7 @@ async function bundleStyleResources(compilation, resourcePlugins) {

await fs.writeFile(new URL(`./${optimizedFileName}`, outputDir), optimizedFileContents);
}
}
});
}

async function bundleApiRoutes(compilation) {
Expand All @@ -300,11 +271,10 @@ async function bundleApiRoutes(compilation) {

if (apiConfigs.length > 0 && apiConfigs[0].input.length !== 0) {
console.info("bundling API routes...");
for (const configIndex in apiConfigs) {
const rollupConfig = apiConfigs[configIndex];
await asyncForEach(apiConfigs, async (rollupConfig) => {
Comment on lines -303 to +274

@KaiPrince KaiPrince Aug 21, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I double-checked, and apiConfigs is an array anyways, so converting from for...in to for...of works fine. No special handling was needed for accessing by index.

const bundle = await rollup(rollupConfig);
await bundle.write(rollupConfig.output);
}
});
}
}

Expand All @@ -326,7 +296,7 @@ async function bundleSsrPages(compilation, optimizePlugins) {
// one pass to generate initial static HTML and to track all combined static resources across layouts
// and before we optimize so that all bundled assets can tracked up front
// would be nice to see if this can be done in a single pass though...
for (const page of ssrPages) {
await asyncForEach(ssrPages, async (page) => {
const { route } = page;
let staticHtml = "<content-outlet></content-outlet>";

Expand All @@ -346,15 +316,15 @@ async function bundleSsrPages(compilation, optimizePlugins) {
await trackResourcesForRoute(staticHtml, compilation, route);

ssrPrerenderPagesRouteMapper[route] = staticHtml;
}
});

// technically this happens in the start of bundleCompilation once
// so might be nice to detect those static assets to see if they have be "de-duped" from bundling here
await bundleScriptResources(compilation);
await bundleStyleResources(compilation, optimizePlugins);

// second pass to link all bundled assets to their resources before optimizing and generating SSR bundles
for (const page of ssrPages) {
await asyncForEach(ssrPages, async (page) => {
const { id, route, pageHref } = page;
const pagePath = new URL(pageHref).pathname.replace(pagesDir.pathname, "./");
const entryFileUrl = new URL(pageHref);
Expand Down Expand Up @@ -422,17 +392,16 @@ async function bundleSsrPages(compilation, optimizePlugins) {
id,
inputPath: normalizePathnameForWindows(entryFileOutputUrl),
});
}
});

const ssrConfigs = await getRollupConfigForSsrPages(compilation, input);

if (ssrConfigs.length > 0 && ssrConfigs[0].input !== "") {
console.info("bundling dynamic pages...");
for (const configIndex in ssrConfigs) {
const rollupConfig = ssrConfigs[configIndex];
await asyncForEach(ssrConfigs, async (rollupConfig) => {
const bundle = await rollup(rollupConfig);
await bundle.write(rollupConfig.output);
}
});
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/lifecycles/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ const generateCompilation = async () => {

if (!(await checkResourceExists(new URL("./graph.json", outputDir)))) {
return Promise.reject(
new Error("No build output detected. Make sure you have run greenwood build"),
new Error("No build output detected. Make sure you have run greenwood build"),
);
}

Expand Down
24 changes: 12 additions & 12 deletions packages/cli/src/lifecycles/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// @ts-nocheck
import fs from "node:fs/promises";
import { checkResourceExists } from "../lib/resource-utils.js";
import { asyncMap } from "../lib/async-utils.js";

const cwd = new URL(`file://${process.cwd()}/`);
const greenwoodPluginsDirectoryUrl = new URL("../plugins/", import.meta.url);
Expand All @@ -9,26 +10,25 @@ const PLUGINS_FLATTENED_DEPTH = 2;
// get and "tag" all plugins provided / maintained by the @greenwood/cli
// and include as the default set, with all user plugins getting appended
const greenwoodPlugins = (
await Promise.all(
await asyncMap(
[
new URL("./copy/", greenwoodPluginsDirectoryUrl),
new URL("./renderer/", greenwoodPluginsDirectoryUrl),
new URL("./resource/", greenwoodPluginsDirectoryUrl),
new URL("./server/", greenwoodPluginsDirectoryUrl),
].map(async (pluginDirectoryUrl) => {
],
async (pluginDirectoryUrl) => {
const files = await fs.readdir(pluginDirectoryUrl);

return await Promise.all(
files.map(async (file) => {
const importUrl = new URL(`./${file}`, pluginDirectoryUrl);
// @ts-expect-error see https://github.com/microsoft/TypeScript/issues/42866
const pluginImport = await import(importUrl);
const plugin = pluginImport[Object.keys(pluginImport)[0]];
return await asyncMap(files, async (file) => {
const importUrl = new URL(`./${file}`, pluginDirectoryUrl);
// @ts-expect-error see https://github.com/microsoft/TypeScript/issues/42866
const pluginImport = await import(importUrl);
const plugin = pluginImport[Object.keys(pluginImport)[0]];

return Array.isArray(plugin) ? plugin : [plugin];
}),
);
}),
return Array.isArray(plugin) ? plugin : [plugin];
});
},
)
)
.flat(PLUGINS_FLATTENED_DEPTH)
Expand Down
Loading
Loading