enhancement/issue 823 ensure greenwood cli and plugins are favoring idiomatic usages of async by KaiPrince · Pull Request #1546 · ProjectEvergreen/greenwood

KaiPrince · 2025-08-04T04:06:54Z

I did a super not-scientific benchmark, and saw a tiny (3s) improvement in test run times. Again, not super robust, but if there's an actual improvement, it would probably have come from the usages of the improved asyncMap in this resource-utils function.

HEAD	Trial 1	Trial 2	Trial 3	Average
Before: `09bca97`	367.43s	354.75s	354.02s	358.73s
After: `5674288`	359.16s	353.10s	353.91s	355.39s

Related Issue

#823

Documentation

N/A

Summary of Changes

make asyncMap run concurrently
replace .map( async () => ... with asyncMap
convert async reduce callbacks to for..in loops

KaiPrince · 2025-08-04T04:07:18Z

I recommend hiding whitespace changes for easier reviewing

KaiPrince · 2025-08-04T04:07:53Z

+      for (const plugin in resourcePlugins) {
        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();
          }
        }


Each stage is pretty much the same, so a future refactoring might be to extract each of the stages and let them share some code through a common function. It would also reduce the size of this massive function.

KaiPrince · 2025-08-04T04:09:15Z

-    return Promise.reject();
+    throw e;


This one is technically a change, because you can't throw nothing syntactically in JS, but I think rejecting with the actual error instead of nothing is better here anyways.

These are not necessary because async functions implicitly wrap their returns in Promises

thescientist13 · 2025-08-05T13:55:17Z

-          });
-        }
+      .map((page) =>
+        (async () => {


Just at a surface glance anyway, (no benchmarking per se) I think this approach may actually get us further away from the idiomatic usage I had in mind, since this seems like it would go from one (`async) function in the map

.map(async (page) => {

to two functions on each loop, all closures, and as an IIFE.

.map((page) => (async () => {

While performance wasn't necessarily the main goal here, I am a little sensitive to adding more overhead and so additional indirection to the code itself. Plus, and what is a reoccuring theme in this codebase that I wanted to fix as part of this issue, is that I think map might have just been a bad choice in the first place, especially with the async added in there. (FWIW I've been trying to get better at avoiding this pattern in newer code 😇 )

Instead here, I think it's fine to break this down into two operations, first filter, then run a for...of

const pagesToRender = compilation.graph.filter(/* ... */) for(const page of pagesToRender) { /* everything that was in the map goes here */ }

No need for the Promise.all ceremony either.

what do you think?

The main difference is that removing the Promise.all means this won't run concurrently anymore. If you're concerned about performance, that's definitely something you'd want to keep in. See this eslint rule: https://eslint.org/docs/latest/rules/no-await-in-loop

As far as replacing .map goes, perhaps you could clarify what you mean by "idiomatic" in this context?

Is 1233214 closer to what you were thinking?

As far as replacing .map goes, perhaps you could clarify what you mean by "idiomatic" in this context?

In the sense that these array methods (map, reduce, filter) are not asynchronous, and so using async callbacks while not an error per se, feels like it goes against the grain of how to apply those functions

The main difference is that removing the Promise.all means this won't run concurrently anymore.
Is 1233214 closer to what you were thinking?

Yeah, I think that example you shared looks good!

I think i like that pattern of using thefor ... of loop and batching up all the promises functions, then firing them all of at the end to maintain the benefits of the concurrency! 💯

I wonder if we should consider adding that ESLint rule to our configuration? Seems like it would help guide us in this refactoring as well as keeping us "honest" going forward?

As I was posting this, I realized I had created such utils for map and filter already for just this reason! So maybe we can just refactor to use these?

https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/src/lib/async-utils.js

https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/src/lib/layout-utils.js#L184

Refactored to use asyncMap. Left the reduce changes as is since an asyncReduce helper won't make much difference.

Looked into adding the eslint rule, but it currently throws 170 errors, so it should be held off for another time.

thanks for checking!

So looking at those cases you shared, e.g.

// plugin-renderer-lit for (const script of scripts) { await import(script); }

In the case of awaits in a for...of loop, that would be OK, as even the ESLint docs acknowledge this would make sense in certain conditions
https://eslint.org/docs/latest/rules/no-await-in-loop#when-not-to-use-it

In many cases the iterations of a loop are not actually independent of each other, and awaiting in the loop is correct

So basically, my main goal here was to refactor the usage of await in map, forEach, etc where await doesn't make sense and was more of convenience, and so those should definitely go away like you've done so far. But for any for...of usages, I am fine to add an ESLint ignore line for those, which is likely what all those cases probably are. I just want to catch and make sure we don't do this going forward for the cases we do care.

What do you think?

thinking of this more, I think it would be fine if there the lint rule was somehow able to just flag the offenses we're refactoring here (e.g. map, reduce, forEach), otherwise I think let's use the rule to make sure we've found all usages and refactored those. at least with all currant usages cleaned up, then I think this PR would be good to go.

thescientist13 · 2025-08-05T14:02:42Z

-      return Promise.reject(
-        new Error("No build output detected.  Make sure you have run greenwood build"),
-      );
+      throw new Error("No build output detected. Make sure you have run greenwood build");


yeah, I guess technically these produce the same outcome from what I read about comparing Promise.reject vs throw but since all these lifecycles are run in the context of an asynchronous function, would the Promise methods be more meaningful / accurate?
https://github.com/ProjectEvergreen/greenwood/blob/master/packages/cli/src/index.js#L3

Either way, I think it's OK to leave as it was, just to avoid the risk having this PR not try and do too many things at once, since I think we'll have our work cut out for us with all the existing refactoring at hand and reading / reviewing that. 😅

As far as being "idiomatic" goes, returning unwrapped values and throw-ing is more common when using async/await functions. That's why I included it in this PR, but I can take it out no problem. I was just trying to get it all in one go.

Ok, sounds good! As long as the outward behavior is the same (to the user), I think this is fine.

thescientist13 · 2025-08-05T14:03:36Z

-      );
-    }),
+    ].map((pluginDirectoryUrl) =>
+      (async () => {


same as my comment above, we can just choose not to use a .map

thescientist13 · 2025-08-05T14:05:27Z

        customConfig.workspace = workspace;
      } else {
-        return Promise.reject(
+        throw new Error(


same as my comment above, let's avoid any additional refactorings in this PR not directly related to fixing all the loops and things just to try and keep the diff as small and focused as possible . (we can always just open a separate PR for this if we feel strongly about it)

thescientist13 · 2025-08-05T14:07:29Z

  await Promise.all(
-    files.map(
-      async (f) =>
+    files.map((f) =>


same here, no need to use a .map from what I can see. Also, after we merge #1543, we'll have access to fs.glob so I think we won't even need this internal function anymore

thescientist13 · 2025-08-05T14:11:18Z

-
-      return Promise.resolve();
-    }),
+    pages.map((page) =>


This seems like it could easily just be a for ... of and just get rid of the Promise.all + map entirely

likewise this would no longer run concurrently if we removed the promise.all.

thescientist13 · 2025-08-05T14:15:56Z

      (await getDevServer(this.compilation)).listen(offsetPort, async () => {
        console.info(`Started puppeteer prerender server at http://localhost:${offsetPort}`);
      });
-    } else {


are we sure this should / needs to be deleted? I would test with yarn develop but I think it's OK to leave this one out of this PR since it's a little out of scope, IMO.

It's useless statement isn't it?

Async functions always return a promise. If the return value of an async function is not explicitly a promise, it will be implicitly wrapped in a promise.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/async_function

thescientist13 · 2025-08-05T14:16:30Z


 const getPagesFromGraph = async (root, query, context) => {
-  return Promise.resolve(context.graph);
+  return context.graph;


this is probably fine, but as mentioned, maybe better to take all these Promise clean up tasks into their own PR?

thescientist13 · 2025-08-05T14:17:23Z

-        pages.map(async (page) => {
-          const { route } = page;
-          console.info("prerendering page...", route);
+        pages.map((page) =>


same re: refactoring .map

thescientist13 · 2025-08-05T14:18:12Z

+    return this.liveReloadServer.watch(userWorkspace, async () => {
      console.info(`Now watching directory "${userWorkspace}" for changes.`);
-      return Promise.resolve(true);
+      return true;


this old documentation code, so no need to change this

thescientist13 · 2025-08-05T14:21:31Z

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


nice, this looks very elegant! 🤓

thescientist13

This is a great start, thank you so much!

My only reoccurring feedback as you'll see is that my hope was to avoid .map(async) usage, and so "doubling" down in some cases here feels like more work / overhead / code then something more straightforward like a for ... of loop, and ultimately less idiomatic. So I think in those cases if we just refactor that out like in one of my snippets, we should be good there.

Also, I can understand some of the Promise vs throw changes being async adjacent but I think it would probably best to isolate those to their own PR, just to help keep this one focused on just the loops and stuff. (the nature of this refactoring has a lot of diffs, so a lot to read through)

Lastly, I think I found one last case of an async map, which should be easy enough to tweak

Also, if you don't mind commenting on #823 I would like to assign it to you so it's easier to see on our board that someone is working on.

Thanks again and let me know if you have any questions / clarifications.

This reverts commit 5aa3439.

This reverts commit 1233214.

KaiPrince

I originally avoided using the asyncMap function, because I thought we were trying to move away from it to be more "idiomatic". I looked up the definition of idiomatic to understand what was intended in the issue.

An idiomatic way of writing some code is when you write it in a very specific way because of language-specific idioms that other languages don't have.

https://softwareengineering.stackexchange.com/a/94567

Since javascript uses a lot of functional style and async/await syntax, I was trying to lean into that. I've updated this PR based on your feedback. The downside is now lots of files depend on the custom asyncMap definition, but the upside is the rest of the code is slightly simpler.

KaiPrince · 2025-08-11T20:16:08Z

-    mappedItems.push(await mapper(item));
+    promises.push(mapper(item));


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.

KaiPrince · 2025-08-11T20:16:57Z

 }

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


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

KaiPrince · 2025-08-11T20:18:11Z

-        }
+  const pages = compilation.graph.filter(
+    (page) =>
+      !page.isSSR || (page.isSSR && page.prerender) || (page.isSSR && compilation.config.prerender),


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

KaiPrince · 2025-08-11T20:19:19Z

-        for (const plugin of plugins) {
-          if (plugin.shouldOptimize && (await plugin.shouldOptimize(url, response.clone()))) {
-            const currentResponse = await plugin.optimize(url, response.clone());
+  await asyncMap(pages, async (page) => {


One could argue that this would be better off as asyncForEach, since the callbacks have side-effects and we're not actually using the return values.

that makes sense, I would be open to an asyncForEach

thescientist13

This coming along great!

I would be open to adding the linting rule as I think most of the violations flagged are probably just going to be in for..of loops and so I think those are fine to add an ESLint ignore line for

In regards to your comment here

The downside is now lots of files depend on the custom asyncMap definition, but the upside is the rest of the code is slightly simpler.

I am OK with that tradeoff. The functions are pretty simple and well named, and I think the value of having a consistent way to apply such logic is ideal for our use cases.

Also, there is one minor merge conflict but should be easy to resolve, just remove passing the route param here to createOutputDirectory

Thanks for doing the benchmarking! 💯

Prior to the 1.0 release, I would like to revisit some of the work I've done as part of #970 in this benchmarking repo someone created for frameworks a while ago
https://github.com/thescientist13/bench-framework-markdown

…-823-ensure-Greenwood-CLI-and-plugins-are-favoring-idiomatic-usages-of-async

Set to "warn" because there are a lot of reduce exceptions and commenting them each would be tedious. Resolved some by switching to the new asyncForEach function

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

thescientist13 · 2025-08-21T02:30:33Z

      "no-only-tests/no-only-tests": "error",
      "import/no-unresolved": "off",
      "import/enforce-node-protocol-usage": ["error", "always"],
+      "no-await-in-loop": "warn",


my concern with adding this rule with no "constraint" on this is that now we get a ton of warnings

Details

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/commands/eject.js 11:5 warning Unexpected `await` inside a loop no-await-in-loop
/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/config/rollup.config.js
78:42 warning Unexpected await inside a loop no-await-in-loop
79:32 warning Unexpected await inside a loop no-await-in-loop
90:38 warning Unexpected await inside a loop no-await-in-loop
91:24 warning Unexpected await inside a loop no-await-in-loop
98:14 warning Unexpected await inside a loop no-await-in-loop
100:24 warning Unexpected await inside a loop no-await-in-loop
107:14 warning Unexpected await inside a loop no-await-in-loop
109:24 warning Unexpected await inside a loop no-await-in-loop
114:41 warning Unexpected await inside a loop no-await-in-loop
115:24 warning Unexpected await inside a loop no-await-in-loop
145:38 warning Unexpected await inside a loop no-await-in-loop
287:40 warning Unexpected await inside a loop no-await-in-loop
288:30 warning Unexpected await inside a loop no-await-in-loop
293:38 warning Unexpected await inside a loop no-await-in-loop
294:24 warning Unexpected await inside a loop no-await-in-loop
302:14 warning Unexpected await inside a loop no-await-in-loop
304:24 warning Unexpected await inside a loop no-await-in-loop
312:14 warning Unexpected await inside a loop no-await-in-loop
314:24 warning Unexpected await inside a loop no-await-in-loop
357:31 warning Unexpected await inside a loop no-await-in-loop
363:38 warning Unexpected await inside a loop no-await-in-loop
364:30 warning Unexpected await inside a loop no-await-in-loop
637:22 warning Unexpected await inside a loop no-await-in-loop
639:32 warning Unexpected await inside a loop no-await-in-loop
646:22 warning Unexpected await inside a loop no-await-in-loop
648:32 warning Unexpected await inside a loop no-await-in-loop
655:22 warning Unexpected await inside a loop no-await-in-loop
657:32 warning Unexpected await inside a loop no-await-in-loop
663:19 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lib/async-utils.js
6:21 warning Unexpected await inside a loop no-await-in-loop
21:22 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lib/layout-utils.js
23:11 warning Unexpected await inside a loop no-await-in-loop
165:40 warning Unexpected await inside a loop no-await-in-loop
198:40 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lib/resource-utils.js
114:9 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lib/walker-package-ranger.js
235:15 warning Unexpected await inside a loop no-await-in-loop
255:11 warning Unexpected await inside a loop no-await-in-loop
300:14 warning Unexpected await inside a loop no-await-in-loop
304:11 warning Unexpected await inside a loop no-await-in-loop
309:13 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/bundle.js
26:39 warning Unexpected await inside a loop no-await-in-loop
27:18 warning Unexpected await inside a loop no-await-in-loop
30:36 warning Unexpected await inside a loop no-await-in-loop
31:18 warning Unexpected await inside a loop no-await-in-loop
44:35 warning Unexpected await inside a loop no-await-in-loop
45:50 warning Unexpected await inside a loop no-await-in-loop
110:7 warning Unexpected await inside a loop no-await-in-loop
144:41 warning Unexpected await inside a loop no-await-in-loop
145:37 warning Unexpected await inside a loop no-await-in-loop
193:13 warning Unexpected await inside a loop no-await-in-loop
194:9 warning Unexpected await inside a loop no-await-in-loop
205:22 warning Unexpected await inside a loop no-await-in-loop
224:18 warning Unexpected await inside a loop no-await-in-loop
249:18 warning Unexpected await inside a loop no-await-in-loop
274:18 warning Unexpected await inside a loop no-await-in-loop
284:31 warning Unexpected await inside a loop no-await-in-loop
292:7 warning Unexpected await inside a loop no-await-in-loop
305:22 warning Unexpected await inside a loop no-await-in-loop
306:7 warning Unexpected await inside a loop no-await-in-loop
333:20 warning Unexpected await inside a loop no-await-in-loop
334:20 warning Unexpected await inside a loop no-await-in-loop
335:20 warning Unexpected await inside a loop no-await-in-loop
337:20 warning Unexpected await inside a loop no-await-in-loop
338:9 warning Unexpected await inside a loop no-await-in-loop
346:7 warning Unexpected await inside a loop no-await-in-loop
372:20 warning Unexpected await inside a loop no-await-in-loop
373:9 warning Unexpected await inside a loop no-await-in-loop
377:13 warning Unexpected await inside a loop no-await-in-loop
378:9 warning Unexpected await inside a loop no-await-in-loop
387:7 warning Unexpected await inside a loop no-await-in-loop
433:24 warning Unexpected await inside a loop no-await-in-loop
434:9 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/copy.js
46:30 warning Unexpected await inside a loop no-await-in-loop
48:30 warning Unexpected await inside a loop no-await-in-loop
49:11 warning Unexpected await inside a loop no-await-in-loop
53:11 warning Unexpected await inside a loop no-await-in-loop
67:23 warning Unexpected await inside a loop no-await-in-loop
73:9 warning Unexpected await inside a loop no-await-in-loop
75:9 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/graph.js
73:10 warning Unexpected await inside a loop no-await-in-loop
74:10 warning Unexpected await inside a loop no-await-in-loop
77:27 warning Unexpected await inside a loop no-await-in-loop
91:38 warning Unexpected await inside a loop no-await-in-loop
110:33 warning Unexpected await inside a loop no-await-in-loop
162:28 warning Unexpected await inside a loop no-await-in-loop
175:29 warning Unexpected await inside a loop no-await-in-loop
178:13 warning Unexpected await inside a loop no-await-in-loop
347:20 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/prerender.js
23:32 warning Unexpected await inside a loop no-await-in-loop
24:18 warning Unexpected await inside a loop no-await-in-loop
40:8 warning Unexpected await inside a loop no-await-in-loop
42:42 warning Unexpected await inside a loop no-await-in-loop
45:36 warning Unexpected await inside a loop no-await-in-loop
46:42 warning Unexpected await inside a loop no-await-in-loop
92:16 warning Unexpected await inside a loop no-await-in-loop
92:23 warning Unexpected await inside a loop no-await-in-loop
93:12 warning Unexpected await inside a loop no-await-in-loop
93:19 warning Unexpected await inside a loop no-await-in-loop
112:23 warning Unexpected await inside a loop no-await-in-loop
117:12 warning Unexpected await inside a loop no-await-in-loop
142:5 warning Unexpected await inside a loop no-await-in-loop
143:5 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/lifecycles/serve.js
70:36 warning Unexpected await inside a loop no-await-in-loop
71:27 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/loader.js
36:8 warning Unexpected await inside a loop no-await-in-loop
41:24 warning Unexpected await inside a loop no-await-in-loop
47:32 warning Unexpected await inside a loop no-await-in-loop
51:44 warning Unexpected await inside a loop no-await-in-loop
59:8 warning Unexpected await inside a loop no-await-in-loop
66:11 warning Unexpected await inside a loop no-await-in-loop
71:36 warning Unexpected await inside a loop no-await-in-loop
75:44 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/src/plugins/resource/plugin-standard-html.js
74:31 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/cli/test/cases/build.plugins.adapter/generic-adapter.js
36:21 warning Unexpected await inside a loop no-await-in-loop
40:5 warning Unexpected await inside a loop no-await-in-loop
43:7 warning Unexpected await inside a loop no-await-in-loop
53:5 warning Unexpected await inside a loop no-await-in-loop
58:7 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-aws/src/index.js
84:21 warning Unexpected await inside a loop no-await-in-loop
88:5 warning Unexpected await inside a loop no-await-in-loop
91:5 warning Unexpected await inside a loop no-await-in-loop
99:7 warning Unexpected await inside a loop no-await-in-loop
111:5 warning Unexpected await inside a loop no-await-in-loop
113:5 warning Unexpected await inside a loop no-await-in-loop
118:7 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-netlify/src/index.js
107:21 warning Unexpected await inside a loop no-await-in-loop
112:5 warning Unexpected await inside a loop no-await-in-loop
115:5 warning Unexpected await inside a loop no-await-in-loop
123:7 warning Unexpected await inside a loop no-await-in-loop
128:5 warning Unexpected await inside a loop no-await-in-loop
149:5 warning Unexpected await inside a loop no-await-in-loop
151:5 warning Unexpected await inside a loop no-await-in-loop
156:7 warning Unexpected await inside a loop no-await-in-loop
161:5 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-vercel/src/index.js
100:21 warning Unexpected await inside a loop no-await-in-loop
104:5 warning Unexpected await inside a loop no-await-in-loop
107:5 warning Unexpected await inside a loop no-await-in-loop
115:7 warning Unexpected await inside a loop no-await-in-loop
127:5 warning Unexpected await inside a loop no-await-in-loop
129:5 warning Unexpected await inside a loop no-await-in-loop
134:7 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-vercel/test/cases/build.config.base-path/build.config.base-path.spec.js
119:28 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-vercel/test/cases/build.default.options-runtime/build.default.options-runtime.spec.js
119:28 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-adapter-vercel/test/cases/build.default/build.default.spec.js
135:28 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-css-modules/src/index.js
51:32 warning Unexpected await inside a loop no-await-in-loop
52:18 warning Unexpected await inside a loop no-await-in-loop
59:8 warning Unexpected await inside a loop no-await-in-loop
61:18 warning Unexpected await inside a loop no-await-in-loop
68:8 warning Unexpected await inside a loop no-await-in-loop
70:18 warning Unexpected await inside a loop no-await-in-loop
168:5 warning Unexpected await inside a loop no-await-in-loop
222:11 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-graphql/src/schema/schema.js
43:53 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-include-html/src/index.js
29:33 warning Unexpected await inside a loop no-await-in-loop
50:42 warning Unexpected await inside a loop no-await-in-loop
51:33 warning Unexpected await inside a loop no-await-in-loop
51:51 warning Unexpected await inside a loop no-await-in-loop

/Users/owenbuckley/Workspace/project-evergreen/greenwood/packages/plugin-renderer-lit/src/execute-route-module.js
26:7 warning Unexpected await inside a loop no-await-in-loop

What I was trying to callout in my comment was that using await in a for ... of is fine. So unless we can limit the linting to just map, filter, etc then I worry about all the extra noise this adds. The only other option is to add an eslint ignore to all those locations, which seemed pretty tedious.

hmm, I realize i kind of left an unfinished sentence here, my apologies 😞

I would be open to adding the linting rule as I think most of the violations flagged are probably just going to be in for..of loops and so I think those are fine to add an ESLint ignore line for

So yeah, my above comment was what I trying to say, so sorry about that mixup

Yeah it's a lot. That's what I was trying to say in my other comment. We'd have to add eslint-disable comments to a lot of places, or just leave it as a warn (which as you say can get pretty noisy).

I thought you confirmed you wanted it for keeping us "honest", but no problem I'll just revert this config change for now. If we decide later we want to add all those eslint-ignore comments, I can open a separate PR.

Yes, apologies I didn't finish my full thought in that most recent comment, but I was trying to say if an eslint rule is added, it should (ideally) be done with the ability to carve out an exception for the use cases we do want it for (e.g. for...of, for..in)

So the options would be

Enable with error and add // eslint-disable-line no-await-in-loop everywhere its allowed (or see if we can just have it error on map, filter, etc

Just leave things as they are (and just fix the merge conflict, which you done, which is great!)

So I was leaning towards option #2 depending if the carve out was feasible, or if we wanted to sprinkle in all those ignore lines.

Thoughts?

Like I said I am good with things right now, I've personally broken the habit of the async maps, filters etc so I am at least trying to keep myself honest now 😇

Also, you might want to do one more rebase since I bumped the Node version and so GitHub Actions checks have been updated accordingly. Then we can run the final checks on your PR. 👍

I started adding eslint-ignore comments initially, but I think I hit a snag in the rollup config file. Let's do option 2 for now, and I'll start a follow-up PR to see where option 1 takes us.

PR has been updated from master 👍

KaiPrince · 2025-08-21T02:27:52Z

-    for (const configIndex in apiConfigs) {
-      const rollupConfig = apiConfigs[configIndex];
+    await asyncForEach(apiConfigs, async (rollupConfig) => {


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.

KaiPrince · 2025-08-21T02:40:34Z

      "no-only-tests/no-only-tests": "error",
      "import/no-unresolved": "off",
      "import/enforce-node-protocol-usage": ["error", "always"],
+      "no-await-in-loop": "warn",


Yeah it's a lot. That's what I was trying to say in my other comment. We'd have to add eslint-disable comments to a lot of places, or just leave it as a warn (which as you say can get pretty noisy).

I thought you confirmed you wanted it for keeping us "honest", but no problem I'll just revert this config change for now. If we decide later we want to add all those eslint-ignore comments, I can open a separate PR.

…-and-plugins-are-favoring-idiomatic-usages-of-async

thescientist13 · 2025-08-21T14:01:18Z

+  return Promise.all(promises);
+}
+
+async function asyncForEach(items, callback) {


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?

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.

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! 🙌

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

thescientist13 · 2025-08-21T14:03:27Z

  const { outputDir } = compilation.context;

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


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.

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.

thescientist13

Great stuff, thanks for much for spearheading this change, and also getting some nice little perf boosts along the way!

I'll be planning to cut another alpha release in our current release line this weekend so will get this merged in for that. 🚀

KaiPrince · 2025-08-22T11:37:13Z

I don’t have permission to merge it so you gotta click the button

KaiPrince commented Aug 4, 2025

View reviewed changes

KaiPrince added 2 commits August 4, 2025 12:34

chore: refactor async callbacks

a805a36

chore: remove promise.resolve/reject in async functions

5aa3439

These are not necessary because async functions implicitly wrap their returns in Promises

KaiPrince force-pushed the enhancement/issue-823-ensure-Greenwood-CLI-and-plugins-are-favoring-idiomatic-usages-of-async branch from 941f207 to f92760d Compare August 4, 2025 19:35

KaiPrince marked this pull request as ready for review August 4, 2025 19:36

thescientist13 linked an issue Aug 5, 2025 that may be closed by this pull request

ensure Greenwood CLI and plugins are favoring idiomatic usages of async #823

Closed

5 tasks

thescientist13 reviewed Aug 5, 2025

View reviewed changes

Comment thread packages/cli/src/lifecycles/graph.js Outdated

thescientist13 reviewed Aug 5, 2025

View reviewed changes

KaiPrince added 2 commits August 6, 2025 20:28

Revert "chore: remove promise.resolve/reject in async functions"

2a8fcf8

This reverts commit 5aa3439.

refactor one function

1233214

KaiPrince force-pushed the enhancement/issue-823-ensure-Greenwood-CLI-and-plugins-are-favoring-idiomatic-usages-of-async branch from f92760d to 1233214 Compare August 7, 2025 03:28

KaiPrince mentioned this pull request Aug 7, 2025

ensure Greenwood CLI and plugins are favoring idiomatic usages of async #823

Closed

5 tasks

thescientist13 added enhancement Improve something existing (e.g. no docs, new APIs, etc) CLI Plugins Greenwood Plugins labels Aug 9, 2025

thescientist13 changed the title ~~Enhancement/issue 823 ensure greenwood cli and plugins are favoring idiomatic usages of async~~ enhancement/issue 823 ensure greenwood cli and plugins are favoring idiomatic usages of async Aug 9, 2025

KaiPrince added 3 commits August 11, 2025 11:45

use clone consistently

f3575b8

Revert "refactor one function"

b730cbd

This reverts commit 1233214.

Update asyncMap

4ecc2bf

use asyncMap

5674288

KaiPrince commented Aug 11, 2025

View reviewed changes

KaiPrince requested a review from thescientist13 August 11, 2025 20:29

thescientist13 reviewed Aug 16, 2025

View reviewed changes

use asyncForEach

b65ee02

thescientist13 mentioned this pull request Aug 16, 2025

feat(plugins): #1247 standalone markdown plugin #1457

Merged

27 tasks

KaiPrince added 4 commits August 18, 2025 21:47

Merge remote-tracking branch 'upstream/master' into enhancement/issue…

6fcbf11

…-823-ensure-Greenwood-CLI-and-plugins-are-favoring-idiomatic-usages-of-async

Add eslint await in loop rule

41e0acc

Set to "warn" because there are a lot of reduce exceptions and commenting them each would be tedious. Resolved some by switching to the new asyncForEach function

fix: revert merged response .clone cleanup

7e3f0c8

use return await for better stack trace

a5af458

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

thescientist13 reviewed Aug 21, 2025

View reviewed changes

KaiPrince added 3 commits August 20, 2025 19:32

await forEach

2159832

await more forEach

80afa74

revert no await in loop eslint rule

2a591b0

KaiPrince commented Aug 21, 2025

View reviewed changes

Merge branch 'master' into enhancement/issue-823-ensure-Greenwood-CLI…

7f2339a

…-and-plugins-are-favoring-idiomatic-usages-of-async

thescientist13 reviewed Aug 21, 2025

View reviewed changes

add return await for better stack trace

6da1e3b

KaiPrince requested a review from thescientist13 August 21, 2025 17:10

thescientist13 approved these changes Aug 21, 2025

View reviewed changes

thescientist13 merged commit e6c484d into ProjectEvergreen:master Aug 23, 2025
10 checks passed

thescientist13 mentioned this pull request Sep 13, 2025

resolve max event listeners warning #1575

Closed

KaiPrince deleted the enhancement/issue-823-ensure-Greenwood-CLI-and-plugins-are-favoring-idiomatic-usages-of-async branch March 15, 2026 07:39

		mappedItems.push(await mapper(item));
		promises.push(mapper(item));

Uh oh!

Conversation

KaiPrince commented Aug 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Related Issue

Documentation

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thescientist13 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thescientist13 Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thescientist13 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thescientist13 Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thescientist13 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiPrince left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KaiPrince commented Aug 4, 2025 •

edited

Loading

thescientist13 Aug 5, 2025 •

edited

Loading

thescientist13 Aug 9, 2025 •

edited

Loading

thescientist13 Aug 5, 2025 •

edited

Loading

thescientist13 Aug 5, 2025 •

edited

Loading

thescientist13 left a comment •

edited

Loading

thescientist13 Aug 21, 2025 •

edited

Loading