-
Notifications
You must be signed in to change notification settings - Fork 15
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.
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
Changes from 8 commits
a805a36
5aa3439
2a8fcf8
1233214
f3575b8
b730cbd
4ecc2bf
5674288
b65ee02
6fcbf11
41e0acc
7e3f0c8
a5af458
2159832
80afa74
2a591b0
7f2339a
6da1e3b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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())); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice, this looks very elegant! 🤓 |
||
|
|
||
| if (prerenderPlugin.executeModuleUrl) { | ||
| await preRenderCompilationWorker(compilation, prerenderPlugin); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,14 +14,15 @@ async function asyncFilter(arr, cb) { | |
| } | ||
|
|
||
| // https://stackoverflow.com/a/71278238/417806 | ||
| // Constraint: mapper functions must not depend on each other | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Promise.all(promises); | ||
| } | ||
|
|
||
| export { asyncFilter, asyncMap }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ import { | |
| import path from "node:path"; | ||
| import { rollup } from "rollup"; | ||
| import { pruneGraph } from "../lib/content-utils.js"; | ||
| import { asyncMap } from "../lib/async-utils.js"; | ||
|
|
||
| async function interceptPage(url, request, plugins, body) { | ||
| let response = new Response(body, { | ||
|
|
@@ -115,45 +116,41 @@ async function cleanUpResources(compilation) { | |
| 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), | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 asyncMap(pages, async (page) => { | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One could argue that this would be better off as
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that makes sense, I would be open to an |
||
| 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) { | ||
|
|
@@ -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) { | ||
|
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(); | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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