Skip to content
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

fix: make redirects work as intended #52

Closed
wants to merge 1 commit into from
Closed

fix: make redirects work as intended #52

wants to merge 1 commit into from

Conversation

flakey5
Copy link
Member

@flakey5 flakey5 commented Oct 18, 2023

Issue #50 showed that redirects aren't working properly. They go off of the url's path, which introduces a couple of problems. For example, when testing /dist/latest-v20.x, the worker checks if nodejs/dist/latest/v20.x is in the REDIRECT_MAP object.

This is in a pretty rough testing state still, but I wanted to open the conversation on this and the approach we want to take

I got rewrites working but it's not exactly nice (see diff). There are a few takeaways I got from tinkering that I think we should keep in mind:

  1. The redirectPaths.json file has the bucket path as the key, so I think we should convert the url to the bucket path and then check if the bucket path is in the REDIRECT_MAP rather than checking for the url (eliminates possibility of looking for redirects for paths such as nodejs/dist/...)
  2. src: automatically rewrite latests and docs links #39 noticed this but restating since I think it's noteworthy. We really don't have any paths deeper than 3 directories that trigger a rewrite. They're all paths such as /download/release/latest/ or /docs/latest or /api.

The code in the diff is working and something we could go with, but I wanted to get others' opinions on it and any other possible approaches before opening it for review.

Fixes #50

@ovflowd
Copy link
Member

ovflowd commented Oct 19, 2023

Can we simply make it a hot-fix for #50 and then open the discussion on another issue for potential improvement paths? It's just that we want to unblock the release of this 👀

@flakey5
Copy link
Member Author

flakey5 commented Oct 19, 2023

@MoLow fixed in #53

@flakey5 flakey5 closed this Oct 19, 2023
@flakey5 flakey5 deleted the flakey5/50 branch November 2, 2023 03:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

latest-v20.x shows two releases
2 participants