nodejs / release-cloudflare-worker

Infra for serving Node.js downloads and documentation.
https://nodejs.org/dist
MIT License
22 stars 5 forks source link

fix: use same directory listing format as nginx #65

Closed flakey5 closed 10 months ago

flakey5 commented 10 months ago

This PR adopts similar directory listing structure as NGINX to keep existing crawlers and scripts working.

jbergstroem commented 10 months ago

~Is there a context to the "fix"? Is the html structure expected for parsing reasons?~

Never mind, just saw the linked issue.

ovflowd commented 10 months ago

nginx uses columns with fixed length: 50 chars for the file name 1 space date 20 chars for the file size

The name is truncated if it's too long: direct.nodejs.org/download/test The size is written in bytes, without a unit

I'm not sure we should limit to 50 characters, I don't think this is an issue for us.

ovflowd commented 10 months ago

@flakey5 can you also add an screenshot on some directory listings how it'd look like?

flakey5 commented 10 months ago

@flakey5 can you also add an screenshot on some directory listings how it'd look like?

image

I wasn't able to figure out how exactly we should do the spacing. We could do something similar to what nginx does re @targos 's comment, it should work

ovflowd commented 10 months ago

We can try it. Also would love if dark theme kept working.

flakey5 commented 10 months ago

This should be good to go, the only difference between the nginx listing & the worker should be that directories don't have a value for the last modified part

image

Also would love if dark theme kept working.

Same but will probably need to be something we revisit

ovflowd commented 10 months ago

How we deploy to staging? 🤔

targos commented 10 months ago

Isn't it done automatically when the PR is merged on main? I see deploy workflow runs on each commit here: https://github.com/nodejs/release-cloudflare-worker/commits/main/

ovflowd commented 10 months ago

Isn't it done automatically when the PR is merged on main? I see deploy workflow runs on each commit here: main (commits)

Oh yes, it is :)

flakey5 commented 10 months ago

Should I merge to test in staging?

flakey5 commented 10 months ago

There still appears to be some differences

image

ovflowd commented 10 months ago

There still appears to be some differences

image

Ugh, it should give v20.0.9.0... I wonder what is different.

richardlau commented 10 months ago

Quotes? (Single vs double)

flakey5 commented 10 months ago

Quotes? (Single vs double)

Yeah that appears to be the issue, thanks for catching! https://github.com/nodejs/release-cloudflare-worker/pull/76