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: transfered misplaced blog pages that caused broken links #6644

Merged
merged 3 commits into from
May 13, 2024

Conversation

mAmineChniti
Copy link
Contributor

@mAmineChniti mAmineChniti commented Apr 10, 2024

Description

Transfered v5-to-v7.md and update-v8-5.4.md from community directory to announcements directory, the category in those files is announcements and being misplaced cause broken links

Validation

I've ran npx turbo serve and checked the links, i've gone through the blog directory and verified that there are no more misplaced blog pages in other categories
update-v8-5.4.md

mstsc_fYJSk7uZwD

v5-to-v7.md

mstsc_xhvA1Gubbx

Related Issues

Fixes #6641

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo 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.

@mAmineChniti mAmineChniti requested review from a team as code owners April 10, 2024 23:54
Copy link

vercel bot commented Apr 10, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 13, 2024 11:13am

@reeveng
Copy link

reeveng commented Apr 11, 2024

duplicate of #6643

AugustinMauroy

This comment was marked as off-topic.

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

The problem is not the file name. And it MUST not be changed. This should come from the withSidebarCrossLinks component.

@mAmineChniti
Copy link
Contributor Author

i've tested out the new checks and code that i wrote by temporarily deleting the update-v8-5.4.md from the announcements folder, and heading to http://localhost:3000/en/blog/announcements/nodejs-certified-developer-program like #6641 mentions and now the update-v8-5.4 doesn't render (and all other invalid links don't as well)
NXqWzRTWxN

@mAmineChniti
Copy link
Contributor Author

mAmineChniti commented Apr 12, 2024

@ovflowd @AugustinMauroy i'd like to have your feedback on a couple of ideas to make sure that we don't include blog md that isn't in the right folder:

Idea 1:

what if we add a check in blogData.mjs getFrontMatter where we compare between the category in the filename and the category that we fetch from the file itself like this

const pathnameCategory = dirname(filename).split('/').pop();
if (pathnameCategory === category) {
    // add whats needed
    const publishYear = new Date(date).getUTCFullYear();
    // etc etc
    return {
        title,
        author,
        username,
        date: new Date(date),
        categories,
        slug
    };
} else return {};

then in generateBlogData we do

_readLine.on('close', () => {
    const post = getFrontMatter(filename, rawFrontmatter[filename][1]);
    if (Object.keys(post).length > 0) {
        posts.push(post);
    }
    if (posts.length === filenames.length) {
        resolve({
            categories: [...blogCategories],
            posts
        });
    }
});

Idea 2:

having a check in generateBlogData:

_readLine.on('close', () => {
    const post = getFrontMatter(filename, rawFrontmatter[filename][1]);
    const fileCategory = dirname(filename).split('/').pop();
    if (fileCategory === post.categories[0]) {
        posts.push(post);
    }
    if (posts.length === filenames.length) {
        resolve({
            categories: [...blogCategories],
            posts
        });
    }
});

@ovflowd
Copy link
Member

ovflowd commented Apr 21, 2024

TBH I think all of these solutions are overkill. These specific blog posts seems to be very edge scenarios. IMHO we should simply add redirects to them on our redirects.json file (from the old links to the new ones)

@AugustinMauroy
Copy link
Contributor

TBH I think all of these solutions are overkill. These specific blog posts seems to be very edge scenarios. IMHO we should simply add redirects to them on our redirects.json file (from the old links to the new ones)

Claudio take a look to the issue. the issue from crosslink with category. Not from "miss placed post". Md/mdx file mustn't not change place.

@mAmineChniti
Copy link
Contributor Author

@AugustinMauroy i actually disagree on that, if you open update-v8-5.4.md and v5-to-v7.md their category clearly states announcements but they are placed in the community folder, the slugs being generated are getting the categories from the file itself and not the pathname const slug = `/blog/${category}/${basename(filename, extname(filename))}`; which results in crosslink serving the slug which doesn't correspond to the actual file/content path resulting in the broken links

@mAmineChniti
Copy link
Contributor Author

TBH I think all of these solutions are overkill. These specific blog posts seems to be very edge scenarios. IMHO we should simply add redirects to them on our redirects.json file (from the old links to the new ones)

@ovflowd In that case this PR can be merged safely as it simply puts the blog posts in their right folder according to their category and adds a few minor changes to the withSidebarCrossLinks.tsx which i can revert if you want, there's no need to add anything to the redirects.json, if you open up the files you can clearly see that they have the category of announcements but are placed in the community folder

@unclebay143
Copy link

Any progress here?
cc: @mAmineChniti @AugustinMauroy @ovflowd @aymen94

@mAmineChniti
Copy link
Contributor Author

mAmineChniti commented May 9, 2024

@unclebay143 so far im waiting for feedback to know which direction to take if there's any or if this is ready to be accepted but based on @ovflowd last comment I THINK this PR can be safely merged and achieves its purpose

@ovflowd
Copy link
Member

ovflowd commented May 10, 2024

Sorry, I was out on a company event, I'll try to review this asap.

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

I don't think the changes on components/withSidebarCrossLinks.tsx are needed. The previous logic does pretty much the same, you're just complicating things a bit 😅

I do believe you could keep the check of if sidebarNavigation is not defined or the list of items is empty. But these are edge cases that should genuinely not happen.

+ You're moving files but you didn't add redirects from the OLD links to the new ones. (Please update redirects.json) adding the old links to go to the new ones 🙇

@mAmineChniti
Copy link
Contributor Author

I don't think the changes on components/withSidebarCrossLinks.tsx are needed. The previous logic does pretty much the same, you're just complicating things a bit 😅

I do believe you could keep the check of if sidebarNavigation is not defined or the list of items is empty. But these are edge cases that should genuinely not happen.

  • You're moving files but you didn't add redirects from the OLD links to the new ones. (Please update redirects.json) adding the old links to go to the new ones 🙇

I have reverted the changes to the withSidebarCrossLinks component, for more clarity the files being moved are files that have been placed by mistake previously in the wrong blog category folders for example:

update-v8-5.4.md clearly have the category of announcements but was placed in the community folder
WindowsTerminal_f8XTalDGM9

there's no need for adding it to redirects.json since simply placing it in its rightful folder fixes the issue its slug now matches its filepath and we can now access it as the next blog post from the http://localhost:3000/en/blog/announcements/nodejs-certified-developer-program link and not have a dead link for a an existing blog post

@ovflowd
Copy link
Member

ovflowd commented May 13, 2024

I don't think the changes on components/withSidebarCrossLinks.tsx are needed. The previous logic does pretty much the same, you're just complicating things a bit 😅
I do believe you could keep the check of if sidebarNavigation is not defined or the list of items is empty. But these are edge cases that should genuinely not happen.

  • You're moving files but you didn't add redirects from the OLD links to the new ones. (Please update redirects.json) adding the old links to go to the new ones 🙇

I have reverted the changes to the withSidebarCrossLinks component, for more clarity the files being moved are files that have been placed by mistake previously in the wrong blog category folders for example:

update-v8-5.4.md clearly have the category of announcements but was placed in the community folder WindowsTerminal_f8XTalDGM9

there's no need for adding it to redirects.json since simply placing it in its rightful folder fixes the issue its slug now matches its filepath and we can now access it as the next blog post from the http://localhost:3000/en/blog/announcements/nodejs-certified-developer-program link and not have a dead link for a an existing blog post

Alright! Awesome, thanks for explaining!

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for the contribution and moving the files to the right place 🙇

Copy link

github-actions bot commented May 13, 2024

Lighthouse Results

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

Copy link

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.09% (582/646) 76.1% (172/226) 91.4% (117/128)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 5.402s ⏱️

@ovflowd ovflowd disabled auto-merge May 13, 2024 12:54
@ovflowd ovflowd merged commit 73abd86 into nodejs:main May 13, 2024
15 checks passed
@mAmineChniti mAmineChniti deleted the misplaced-blog-posts branch May 13, 2024 13:08
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.

[Broken Link]: Node.js v7 has updated V8 to 5.4
6 participants