Skip to content

Conversation

@bmuenzenmeyer
Copy link
Contributor

@bmuenzenmeyer bmuenzenmeyer commented Jan 24, 2025

Description

perhaps fixes #7413 - i did it in the GUI only

Validation

test the preview

https://nodejs-org-git-favicon-redirect-maybe-openjs.vercel.app/static/images/favicons/favicon.ico

goes to

https://nodejs-org-git-favicon-redirect-maybe-openjs.vercel.app/static/images/favicons/favicon.png

Related Issues

#7413

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npm run format to ensure the code follows the style guide.
  • I have run npm run test to check if all tests are passing.
  • I have run npx turbo build to check if the website builds without errors.
  • I've covered new added functionality with unit tests if necessary.

Signed-off-by: Brian Muenzenmeyer <brian.muenzenmeyer@gmail.com>
Copilot AI review requested due to automatic review settings January 24, 2025 15:54
@bmuenzenmeyer bmuenzenmeyer requested a review from a team as a code owner January 24, 2025 15:54
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • apps/site/redirects.json: Language not supported

@vercel
Copy link

vercel bot commented Jan 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Jan 24, 2025 3:55pm

@MattIPv4
Copy link
Member

MattIPv4 commented Jan 24, 2025

🤔 Is this the right thing to do? Redirecting from an .ico to a .png feels weird to me? The .png of the same name at the same path already exists, so I feel that if folks want a png they can change the extension they're requesting, but an automatic redirect across file types I'm not so sure about.

@bmuenzenmeyer
Copy link
Contributor Author

Is this the right thing to do? Redirecting from an .ico to a .png feels weird to me? The .png of the same name at the same path already exists, so I feel that if folks want a png they can change the extension they're requesting, but an automatic redirect across file types I'm not so sure about.

I'm happy to close this too. I was trying to do a favor to the reporter of ##7413. our previous rationale was that people should not be hotlinking to the site. the presence of -other- image redirects is proof that we cared about continuity beyond our control at one point or another

@MattIPv4
Copy link
Member

Ah yeah I saw the SVG + PNG ones but had missed that we already have another ico -> png redirect further up in the file:

{
      "source": "/(static/|)favicon.ico",
      "destination": "/static/images/favicons/favicon.png"
},

Given that, I withdraw my concern, there is prior art 👍

@MattIPv4 MattIPv4 changed the title add favicon redirect fix: add additional favicon redirect Jan 24, 2025
Copy link
Member

@canerakdas canerakdas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Jan 26, 2025

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 90 🟢 100 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 100 🟢 100 🟢 91 🔗
/en/about/previous-releases 🟢 100 🟢 100 🟢 100 🟢 92 🔗
/en/download 🟢 98 🟢 100 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 100 🟢 96 🟢 92 🔗

@github-actions
Copy link
Contributor

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 88%
87.47% (740/846) 74.3% (240/323) 86.58% (142/164)

Unit Test Report

Tests Skipped Failures Errors Time
182 0 💤 0 ❌ 0 🔥 5.507s ⏱️

@ovflowd ovflowd added this pull request to the merge queue Jan 26, 2025
Merged via the queue into main with commit 1e51ef6 Jan 26, 2025
16 checks passed
@ovflowd ovflowd deleted the favicon-redirect-maybe branch January 26, 2025 16:20
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.

404 on https://nodejs.org/static/images/favicons/favicon.ico

6 participants