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

Broken links to release downloads on Previous Releases page (missing trailing '/') #6110

Closed
weslord opened this issue Nov 11, 2023 · 9 comments · Fixed by #6111 or nodejs/release-cloudflare-worker#63
Labels

Comments

@weslord
Copy link
Contributor

weslord commented Nov 11, 2023

URL:

https://nodejs.org/en/about/previous-releases

Browser Name:

Firefox

Browser Version:

119.0

Operating System:

Ubuntu 23.10

How to reproduce the issue:

The "Releases" links in the table on https://nodejs.org/en/about/previous-releases lead to a simple 404 page.

For example, the Node.js 18.18.2 row links to
https://nodejs.org/download/release/v18.18.2 <- 404

Looks like there's a missing trailing '/'
https://nodejs.org/download/release/v18.18.2/ <- works

@ovflowd
Copy link
Member

ovflowd commented Nov 11, 2023

I dont think this is a bug on the Website. We recently (last days) switched /download/ endpoint to use a new technology for serving those paths (aka Cloudflare R2 + Cloudflare Workers)

I believe the actual issue lies there cc @flakey5

@flakey5
Copy link
Member

flakey5 commented Nov 11, 2023

Yeah it has to deal with how we detect whether or not a path is pointing to a directory or a file in the worker. For instance with https://nodejs.org/download/release/v18.18.2, it sees the .2 at the end of the url and thinks it's the file extension.

Code for it is here https://github.com/nodejs/release-cloudflare-worker/blob/cb2b5306a9c524ef5bda6c1722ffedf5576937a1/src/util.ts#L209C76-L209C76.

Interestingly someone added to it so that paths such as https://nodejs.org/download/release/latest-v21.x work as intended. I think the fix for this would be to add another heuristic that if the characters after the final . in a path are numbers it's considered a directory

@flakey5
Copy link
Member

flakey5 commented Nov 11, 2023

I think #6111 is still good, one less request hitting the worker since we won't need to redirect

@ovflowd
Copy link
Member

ovflowd commented Nov 11, 2023

Right, but we would be checking if the last "." (Dot) is followed by a number? Is that enough of a check?

@flakey5
Copy link
Member

flakey5 commented Nov 11, 2023

Right, but we would be checking if the last "." (Dot) is followed by a number? Is that enough of a check?

Yeah I'm getting a pr for adding the additional check to the worker ready now

@weslord
Copy link
Contributor Author

weslord commented Nov 11, 2023

Right, but we would be checking if the last "." (Dot) is followed by a number? Is that enough of a check?

Do you serve man pages anywhere?


Interestingly, similar dist/ urls seems to redirect
https://nodejs.org/dist/v18.18.2

@ovflowd
Copy link
Member

ovflowd commented Nov 11, 2023

Right, but we would be checking if the last "." (Dot) is followed by a number? Is that enough of a check?

Do you serve man pages anywhere?


Interestingly, similar dist/ urls seems to redirect

https://nodejs.org/dist/v18.18.2

No. And /dist is not under this new worker yet.

@ljharb
Copy link
Member

ljharb commented Nov 12, 2023

It's fine to change docs so they point to the canonical URL, but old URLs need to work forever - it's a core principle of the web. meaning, the links that lack the trailing slash need to redirect to have the trailing slash, i suppose?

@ljharb
Copy link
Member

ljharb commented Nov 12, 2023

Awesome, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants