-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
feat(redirects): handle grapher redirects in grapher worker #2826
Conversation
const redirects = await env.ASSETS.fetch( | ||
new URL("/grapher/_grapherRedirects.json", url), | ||
{ cf: { cacheTtl: 2 * 60 } } | ||
) |
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.
This is really my biggest question in working on this, what is the best way to efficiently fetch the redirects file, which we're gonna have to await on every request now.
Other options include:
- Storing redirects in Workers KV, somehow
- Uploading the redirects.json as part of the worker (ugh, but that's probably actually the fastest way)
I'm also not entirely sure whether the CF caching header will actually do something meaningful here, but it probably can't hurt also, and maybe it can actually speed things up?
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 was surprised by how fast (uncached) fetch from R2 is, so I'd say that fetching redirects from assets explicitly should be light speed. It could be worth logging latency just to be sure.
(I'm still getting 404 from http://staging-site-master/grapher/life-expectancy-of-women-vs-life-expectancy-of-women that is shown in life expectancy. It could be just that workers don't work on staging?) |
Wrong server, http://staging-site-redirects-handling-worker/grapher/life-expectancy-of-women-vs-life-expectancy-of-women works fine. |
You're right; it was a staging server URL issue (fixed by now). |
Work around Cloudflare's 2000 static redirects limit by handling redirects directly in the worker.
This works by baking a
grapher/_grapherRedirects.json
file as part of the baking process, where all grapher redirects always go.The original
_redirects
file will then only contain WP redirects, plus some hardcoded ones.I checked and we're currently down to just 506 redirects in the file now, so quite some room to spare.
I tried out the following variations and they all work:
/grapher/life-expectancy
goes through/grapher/life-EXPECTANCY
redirects to lowercase/grapher/life-expectancy/
removes trailing slash/grapher/life-expectancy-globally-since-1770
redirects/grapher/number-affected-by-natural-disasters
redirects to an explorer using WP redirectCaveats:
/grapher/thumbnail/life-expectancy-globally-since-1770
for example, it would just fail instead of resolving the redirect itself.The
/grapher/_grapherRedirects.json
file looks like this: