fix Edit on GitHub link leading to wrong URL

[DominusKelvin]

From the below video, its seen that most pages are throwing 404 when the Edit on GitHub link is clicked

before.mp4

This PR resolves the issue by moving the call to the permalink plugin after githubLinks and layouts middleware has been called. Below is the result.

after.mp4

[DominusKelvin]

Seems this needs a little more work to solve as I discovered broken links. Closing it for now.

[DominusKelvin]

After a couple of effort to make the "Edit in GitHub" work when generating the page, I noticed a couple of things:

• We can't call githubLinks early without losing the ability to add it to the footer (which is compiled later on in the layouts() plugin
• The current implementation was buggy when converting back to .md because the permalinks plugin turns learn-and-code.html to learn-and-code/index.html. When we convert this to .md we have learn-and-code/index.md which does not exist on GitHub.

I'd like to see a better solution to this without doing the client-side implementation.

Ideally, if there is a way to get the Files object from metalsmith before they are transformed by both markdown and permalinks, that should do the trick(preferably calling after the layouts plugin has done its job so we can have access to the footer)

[DominusKelvin]

I'm open to any pointers on how to make this work without the client-side JavaScript :)

Thanks.

[MaledongGit]

@DominusKelvin：Thanks for understanding it, that's why my first version was using both client js and server codes.

But now, you've also met the two problems I've met before. So IMO：

We can't call githubLinks early without losing the ability to add it to the footer (which is compiled later on in the layouts() plugin

You're right! We should keep it where it is now.

The current implementation was buggy when converting back to .md because the permalinks plugin turns learn-and-code.html to learn-and-code/index.html. When we convert this to .md we have learn-and-code/index.md which does not exist on GitHub.

We don't know whether we have index.md or something else. So we should have a check whether we really need "index.md" or not by checking the local path, something like this below:

function githubLinks(options) {
  return (files, m, next) => {
    Object.keys(files).forEach((path) => {
      if (!isEditableReg.test(path)) {
        return;
      }

      const file = files[path];
      path = path.replace('.html', '.md').replace(/\\/g, '/');

      const currentUrl = `locale/${options.locale}/${path}`;
      if (!fs.existsSync(currentUrl)) {
        path = path.replace("/index.md","");
        path = path + ".md";
        // console.log(`Current Url is: ${currentUrl}`);
        // console.log(`Path is : ${path}`);
      }

      const url = `https://github.com/nodejs/nodejs.org/edit/main/locale/${options.locale}/${path}`;
    ......
}

Notice I've added the currentUrl for us to compare whether the file really exists or not....Maybe this is a simple solution to it.

[DominusKelvin]

Thanks, @MaledongGit. I'm implementing your suggestions now...

[MaledongGit]

Thanks, @MaledongGit. I'm implementing your suggestions now...

Welcome if any better and simple solution here……

[DominusKelvin]

Yeah, it is better @MaledongGit. I'm thinking if there is a way to future-proof the relative path ../../ but I'm not sure. Since __dirname won't work in this case without a relative path being added.

[DominusKelvin]

But I think it is fine since we are using the same(../../) in some other instances in the codebase. Updating this PR now...

[DominusKelvin]

@MaledongGit. The code works but I don't think fs.existsSync(currentUrl) is catching some cases because paths with index.md is still being replaced

[DominusKelvin]

Found the issue: It has to do with how .existsSync() works with relative path. Using path.resolve(__dirname, ../../locale/${options.locale}/${path}) fixed it.

[DominusKelvin]

@MaledongGit I made a push that should have a complete solution now that works and doesn't require client-side code :)

[MaledongGit]

LGTM fot it.

[MaledongGit]

@DominusKelvin：Anyway, thank you for your help on it :)

[DominusKelvin]

You are welcome @MaledongGit and thanks for leading me on the right path to fix it :)

[yhoungdev]

@DominusKelvin i just learnt something really awesome from this PR

[DominusKelvin]

@DominusKelvin i just learnt something really awesome from this PR

I'm glad it was useful to you :)