Clean up addons injection script and fix bugs in library #227

agjohnson · 2024-10-23T23:32:39Z

There is a lot here to modernize this library and fix a number of bugs:

Sets this up as a package so we can use normal package management to define dependencies.
Adds a test framework to help catch bugs. I also found this necessary just to understand what the worker was doing and verify changes.
Reworks the docker container and image so that wrangler can gracefully catch file changes -- it's not necessary to constantly restart wrangler now.
Switch worker from event based to ESM layout -- don't use fetch event and instead use fetch() exported function
Alter how we are returning Response instances, and when this happens.

Fixes Documentation page is blank readthedocs.org#11693
Fixes searchtools.js override responds as text/plain content type addons#410
Refs https://github.com/readthedocs/readthedocs-ops/issues/1513
Fixes Add tests for Addons injection #225
Does not address Break out Addons injection module to own repository #226 yet

We should still figure out moving these files in - #226

agjohnson · 2024-10-23T23:35:02Z

packages/addons-inject/index.js

GitHub diff is 👎 here

This is better, but not by much:

https://github.com/readthedocs/common/pull/227/files/f2bee0cd0b7e98e75d2abf662f800160db9b7809..bea78fb3e280c1846b8c861a013dcd6523cf3336

agjohnson · 2024-10-23T23:37:39Z

I've tested this against projects with large response pages and the exception is actually caught now, and the response falls back to the origin response. The next change will be trying to catch this through streaming and avoiding buffering the entire request.

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.

packages/addons-inject/Dockerfile

packages/addons-inject/index.js

packages/addons-inject/package.json

stsewd · 2024-10-24T15:55:37Z

packages/addons-inject/tests/wrangler.test.js

ericholscher

I don't love all the explicit test logic in the actual code, but seems it's necessary from the comments around it. Overall this looks similar to what we had, and the logic seems the same. I'm not really knowledgable enough about Workers or JS patterns to review that stuff, but the flow seems reasonable, with a lot more async 😨

packages/addons-inject/index.js

humitos

This looks really good to me. I'm happy we have tests 🧪 now and a pattern to keep adding more 💯

It seems the main change here is the usage of the lower-level API fetch and .passThroughOnException() that allow us to catch errors on a try/catch when transforming the clone()d response. I don't fully understand the details behind this, but I think I get the big picture here.

I left some suggestions and questions that will allow me to understand a little more some missing pieces.

I'm trying to test this locally, but it seems the paths for some of the Docker files are wrong. I'm getting this error when running inv docker.build:

=> ERROR [wrangler 2/6] COPY package.json /usr/src/app/packages/addons-inject/                                                                                                        0.0s
 => ERROR [wrangler 3/6] COPY index.js /usr/src/app/packages/addons-inject/                                                                                                            0.0s
 => ERROR [wrangler 4/6] COPY wrangler.toml /usr/src/app/packages/addons-inject/                                                                                                       0.0s
------                                                                                                                                                                                      
 > [wrangler 2/6] COPY package.json /usr/src/app/packages/addons-inject/:                                                                                                                   
------                                                                                                                                                                                      
------                                                                                                                                                                                      
 > [wrangler 3/6] COPY index.js /usr/src/app/packages/addons-inject/:                                                                                                                       
------                                                                                                                                                                                      
------
 > [wrangler 4/6] COPY wrangler.toml /usr/src/app/packages/addons-inject/:
------
failed to solve: failed to compute cache key: failed to calculate checksum of ref UPQU:UMYX:4FAE:CRS7:GXRC:2DRM:5B5J:CRJO:MLEW:4WNJ:5M75:ZC7N::p64btx7uytedu4ry4kp8bwbzq: "/wrangler.toml": not found

I think we need to use relative paths using readthedocs.org as working path.

dockerfiles/docker-compose.yml

packages/addons-inject/package.json

packages/addons-inject/Dockerfile

packages/addons-inject/index.js

packages/addons-inject/tests/wrangler.test.js

dockerfiles/docker-compose.yml

humitos · 2024-10-28T15:40:19Z

After building the images properly I'm getting this error:

wrangler_1     | node:internal/modules/cjs/loader:1078
wrangler_1     |   throw err;
wrangler_1     |   ^
wrangler_1     | 
wrangler_1     | Error: Cannot find module '/usr/src/app/packages/addons-inject/wrangler'
wrangler_1     |     at Module._resolveFilename (node:internal/modules/cjs/loader:1075:15)
wrangler_1     |     at Module._load (node:internal/modules/cjs/loader:920:27)
wrangler_1     |     at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:81:12)
wrangler_1     |     at node:internal/main/run_main_module:23:47 {
wrangler_1     |   code: 'MODULE_NOT_FOUND',
wrangler_1     |   requireStack: []
wrangler_1     | }
wrangler_1     | 
wrangler_1     | Node.js v18.15.0
wrangler_1 exited with code 1

humitos · 2024-10-28T15:50:49Z

Just a note that before merging this PR we should fix the wrangler Docker container so it works with the pattern we already have and builds correctly.

humitos · 2024-10-28T16:06:26Z

dockerfiles/docker-compose.yml

    networks:
      readthedocs:
+    working_dir: /usr/src/app/packages/addons-inject/
    command: [
      "wrangler",


We need this to be as follows to work properly when executing inv docker.up

Suggested change

"wrangler",

"node_modules/.bin/wrangler",

humitos · 2024-10-28T16:07:05Z

Just a note that before merging this PR we should fix the wrangler Docker container so it works with the pattern we already have and builds correctly.

I fixed this and left the solution as suggestions. I was able to build the Docker images and run it as usual with those changes.

agjohnson · 2024-10-29T03:07:35Z

Review seems settled and the production test is looking much better:

I'm going to merge this and we can figure out the deploy next.

Move addons injection files into package structure

9ec259b

We should still figure out moving these files in - #226

agjohnson commented Oct 23, 2024

View reviewed changes

agjohnson added 3 commits October 23, 2024 16:41

Add supplemental files to support updated addons injection module

f2bee0c

Update injection script

2fcd99f

agjohnson force-pushed the agj/addons-package branch from c1c14e9 to bea78fb Compare October 23, 2024 23:42

agjohnson requested review from ericholscher, humitos and stsewd October 23, 2024 23:47

agjohnson mentioned this pull request Oct 23, 2024

Add tests for Addons injection #225

Closed

stsewd reviewed Oct 24, 2024

View reviewed changes

ericholscher approved these changes Oct 24, 2024

View reviewed changes

packages/addons-inject/index.js Outdated Show resolved Hide resolved

ericholscher reviewed Oct 24, 2024

View reviewed changes

packages/addons-inject/index.js Show resolved Hide resolved

agjohnson added 2 commits October 24, 2024 13:49

Update throw debug header

25211a7

Add more comments on response cloning

0699fa0

humitos approved these changes Oct 28, 2024

View reviewed changes

humitos reviewed Oct 28, 2024

View reviewed changes

dockerfiles/docker-compose.yml Outdated Show resolved Hide resolved

agjohnson mentioned this pull request Oct 28, 2024

Break out Addons injection module to own repository #226

Open

5 tasks

humitos reviewed Oct 28, 2024

View reviewed changes

agjohnson added 6 commits October 28, 2024 14:43

Bump up conditional for debug error throw

ff444b8

Test more response header passthrough

fd1de14

Add test for build.commands builds

3184a3d

Add path to wrangler build context

3c2882a

Fix wrangler path in docker compose

26f6246

Increase logging on wrangler

53a0306

agjohnson merged commit 8291bac into main Oct 29, 2024

agjohnson deleted the agj/addons-package branch October 29, 2024 03:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up addons injection script and fix bugs in library #227

Clean up addons injection script and fix bugs in library #227

agjohnson commented Oct 23, 2024 •

edited

Loading

agjohnson Oct 23, 2024 •

edited

Loading

agjohnson commented Oct 23, 2024

stsewd Oct 24, 2024

ericholscher left a comment

humitos left a comment

humitos commented Oct 28, 2024

humitos commented Oct 28, 2024

humitos Oct 28, 2024

humitos commented Oct 28, 2024

agjohnson commented Oct 29, 2024

Clean up addons injection script and fix bugs in library #227

Clean up addons injection script and fix bugs in library #227

Conversation

agjohnson commented Oct 23, 2024 • edited Loading

agjohnson Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

agjohnson commented Oct 23, 2024

stsewd Oct 24, 2024

Choose a reason for hiding this comment

ericholscher left a comment

Choose a reason for hiding this comment

humitos left a comment

Choose a reason for hiding this comment

humitos commented Oct 28, 2024

humitos commented Oct 28, 2024

humitos Oct 28, 2024

Choose a reason for hiding this comment

humitos commented Oct 28, 2024

agjohnson commented Oct 29, 2024

agjohnson commented Oct 23, 2024 •

edited

Loading

agjohnson Oct 23, 2024 •

edited

Loading