Skip to content

Commit c1c14e9

Browse files
agjohnsonagjohnson
committed
Fix issues with response return order
If `Response.clone()` is used without `await` the resulting instance has missing attributes, like `url = null`. The docs do not indicate that `clone()` is async however, so it's not clear why this is required. With this, and more careful response instance usage, most of the cases of dangling, unused, cloned responses have been removed. Now, any response that is used is cloned right as we use it and evaluate the body. This is important, as `response.body` is ultimately what causes the request to buffer.
1 parent 524189c commit c1c14e9

File tree

6 files changed

+432
-65
lines changed

6 files changed

+432
-65
lines changed

‎packages/addons-inject/.gitignore

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.wrangler/
2+
node_modules/
3+
.tool-versions

‎packages/addons-inject/.prettierignore

+3
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
.wrangler/
2+
node_modules/
3+
package-lock.json

‎packages/addons-inject/index.js

+54-42
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,9 @@
1111

1212
/** Class to wrap all module constants, reused in tests
1313
*
14-
* This is a slightly silly hack because Workers can't export consts, though
15-
* exported classes seem to be fine. This should really be a separate import,
16-
* but this complicates the Worker deployment a little.
14+
* This is a slightly silly hack because Workers can't export strings, though
15+
* exported classes seem to be fine. These should really be strings in a
16+
* separate import, but this complicates the Worker deployment a little.
1717
**/
1818
export class AddonsConstants {
1919
// Add "readthedocs-addons.js" inside the "<head>"
@@ -91,59 +91,69 @@ async function onFetch(request, env, context) {
9191
return response;
9292
}
9393

94-
const responseFallback = response.clone();
95-
9694
try {
9795
const transformed = await transformResponse(response);
98-
// Wait on the transformed text for force errors to evaluate. Timeout errors
99-
// and errors thrown during transform aren't actually raised until the body
100-
// is read
101-
return new Response(await transformed.text(), transformed);
96+
// Wait on the transformed body for errors to evaluate. Timeout errors and
97+
// errors thrown during transform aren't actually raised until the body is
98+
// read. If there was no return, we should return the original response.
99+
if (transformed !== undefined) {
100+
return new Response(await transformed.arrayBuffer(), transformed);
101+
}
102102
} catch (error) {
103103
console.error("Discarding error:", error);
104+
console.debug("Returning original response to avoid blank page");
104105
}
105106

106-
console.debug("Returning original response to avoid blank page");
107-
return responseFallback;
107+
return response;
108108
}
109109

110110
export default {
111111
fetch: onFetch,
112112
};
113113

114-
async function transformResponse(originalResponse) {
115-
const { headers } = originalResponse;
114+
/** Transform HTTP reponses
115+
*
116+
* This supports several versions of element removal and transformation of some
117+
* content that was previously injected into project builds.
118+
*
119+
* Important: wait until the last minute to use a `response.clone()`, as if you
120+
* create a clone and do not use the body, this consumes extra memory in the
121+
* worker.
122+
*
123+
* @param response {Response} - The origin response
124+
* @returns {Response} - If transformed, return a response, otherwise `null`.
125+
**/
126+
async function transformResponse(response) {
127+
const { headers } = response;
116128

117129
// Get the content type of the response to manipulate the content only if it's HTML
118130
const contentType = headers.get("content-type") || "";
119131
const injectHostingIntegrations =
120132
headers.get("x-rtd-hosting-integrations") || "false";
121133
const forceAddons = headers.get("x-rtd-force-addons") || "false";
122-
const httpStatus = originalResponse.status;
134+
const httpStatus = response.status;
123135

124136
// Log some debugging data
125137
console.log(`ContentType: ${contentType}`);
126138
console.log(`X-RTD-Force-Addons: ${forceAddons}`);
127139
console.log(`X-RTD-Hosting-Integrations: ${injectHostingIntegrations}`);
128140
console.log(`HTTP status: ${httpStatus}`);
129141

130-
// Debug mode test operations
142+
// Debug mode for some test cases. This is just for triggering an exception
143+
// from tests. There might be a way to conditionally enable this, but I had
144+
// trouble getting Wrangler vars to work at all.
131145
const throwError = headers.get("X-RTD-Throw-Error");
132146
if (throwError) {
133147
console.log(`Throw error: ${throwError}`);
134148
}
135149

136-
// get project/version slug from headers inject by El Proxito
137-
const projectSlug = headers.get("x-rtd-project") || "";
138-
const versionSlug = headers.get("x-rtd-version") || "";
139-
const resolverFilename = headers.get("x-rtd-resolver-filename") || "";
140-
141-
// Check to decide whether or not inject the new beta addons:
142-
//
143-
// - content type has to be "text/html"
144-
// when all these conditions are met, we remove all the old JS/CSS files and inject the new beta flyout JS
150+
// Get project/version slug from headers inject by El Proxito
151+
const projectSlug = headers.get("X-RTD-Project") || "";
152+
const versionSlug = headers.get("X-RTD-Version") || "";
153+
const resolverFilename = headers.get("X-RTD-Resolver-Filename") || "";
145154

146-
// check if the Content-Type is HTML, otherwise do nothing
155+
// Check to decide whether or not inject Addons library. We only do this for
156+
// `text/html` content types.
147157
if (contentType.includes("text/html")) {
148158
// Remove old implementation of our flyout and inject the new addons if the following conditions are met:
149159
//
@@ -153,7 +163,7 @@ async function transformResponse(originalResponse) {
153163
if (forceAddons === "true" && injectHostingIntegrations === "false") {
154164
let rewriter = new HTMLRewriter();
155165

156-
// Remove by selectors
166+
// Remove by selector lookup
157167
for (const script of AddonsConstants.removalScripts) {
158168
rewriter.on(script, new removeElement());
159169
}
@@ -183,20 +193,21 @@ async function transformResponse(originalResponse) {
183193
),
184194
)
185195
.on("*", {
186-
// This mimics a number of exceptions that can occur in the inner
187-
// request to origin, but mostly a hard to replicate timeout due to
188-
// a large page size.
196+
// This is only used for testing purposes. This is used to mimic an
197+
// error being thrown during the request to origin. There are cases
198+
// that are difficult to model here, like a large page size timing out
199+
// the request.
189200
element(element) {
190201
if (throwError) {
191202
throw new Error("Manually triggered error in transform");
192203
}
193204
},
194205
})
195-
.transform(originalResponse);
206+
.transform(await response.clone());
196207
}
197208
}
198209

199-
// Inject the new addons if the following conditions are met:
210+
// Inject Addons if the following conditions are met:
200211
//
201212
// - header `X-RTD-Hosting-Integrations` is present (added automatically when using `build.commands`)
202213
// - header `X-RTD-Force-Addons` is not present (user opted-in into new beta addons)
@@ -208,23 +219,24 @@ async function transformResponse(originalResponse) {
208219
"head",
209220
new addMetaTags(projectSlug, versionSlug, resolverFilename, httpStatus),
210221
)
211-
.transform(originalResponse);
222+
.transform(await response.clone());
212223
}
213224

214225
// Modify `_static/searchtools.js` to re-enable Sphinx's default search
215226
if (
216227
(contentType.includes("text/javascript") ||
217228
contentType.includes("application/javascript")) &&
218229
(injectHostingIntegrations === "true" || forceAddons === "true") &&
219-
originalResponse.url.endsWith("_static/searchtools.js")
230+
response.url.endsWith("_static/searchtools.js")
220231
) {
221-
console.log("Modifying _static/searchtools.js");
222-
return handleSearchToolsJSRequest(originalResponse);
232+
console.debug("Modifying _static/searchtools.js");
233+
// Note, not a `clone()` call here on purpose, as we create a new Response
234+
// in the function already.
235+
return handleSearchToolsJSRequest(response);
223236
}
224237

225-
// if none of the previous conditions are met,
226-
// we return the response without modifying it
227-
return originalResponse;
238+
// Return null response, don't continue transforming this response
239+
return;
228240
}
229241

230242
class removeElement {
@@ -287,11 +299,11 @@ class addMetaTags {
287299
* initialization of the default Sphinx search. This reverts manipulation and
288300
* restores the original Sphinx search initialization.
289301
*
290-
* @param originalResponse {Response} - Response from origin
302+
* @param response {Response} - Response from origin
291303
* @returns {Response}
292304
**/
293-
async function handleSearchToolsJSRequest(originalResponse) {
305+
async function handleSearchToolsJSRequest(response) {
294306
const { pattern, replacement } = AddonsConstants.replacements.searchtools;
295-
const content = await originalResponse.text();
296-
return new Response(content.replace(pattern, replacement), originalResponse);
307+
const content = await response.text();
308+
return new Response(content.replace(pattern, replacement), response);
297309
}

0 commit comments

Comments
 (0)