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

[new docs site] relative links on URLs with and without a trailing slash #15844

Closed
1 task
Tracked by #117
mdjermanovic opened this issue May 6, 2022 · 16 comments · Fixed by #16046
Closed
1 task
Tracked by #117

[new docs site] relative links on URLs with and without a trailing slash #15844

mdjermanovic opened this issue May 6, 2022 · 16 comments · Fixed by #16046
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly documentation Relates to ESLint's documentation
Projects

Comments

@mdjermanovic
Copy link
Member

Environment

main branch, npm start in the docs directory.

What parser are you using?

Default (Espree)

What did you do?

Open USER GUIDE > Rules > no-cond-assign. Then, in the Related Rules section click no-extra-parens link.

What did you expect to happen?

To open no-extra-parens document.

What actually happened?

The link is broken as it points to rules/no-cond-assign/no-extra-parens.

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

This is caused by writing rules/no-cond-assign.md to rules/no-cond-assign/index.html, unlike the current site where it is rules/no-cond-assign.html. The path on the new site is rules/no-cond-assign/ (with / at the end) as opposed to the current site where it is rules/no-cond-assign (without / at the end) so the relative links in the document are broken.

@mdjermanovic mdjermanovic added bug ESLint is working incorrectly documentation Relates to ESLint's documentation labels May 6, 2022
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage May 6, 2022
@mdjermanovic
Copy link
Member Author

The example with related rules no longer applies since those links now start with / , but we'll still have a problem with relative links.

For example, links on this page work well: https://new.eslint.org/docs/user-guide/configuring/

But, if user opens this (no trailing slash): https://new.eslint.org/docs/user-guide/configuring

the underlying docs-eslint.netlify.app redirects to a URL that has a trailing slash, but new.eslint.org does not, and all relative links on the page are broken (check Environments, Globals...)

In addition to broken links, this might affect SEO.

Related issues:

@amareshsm
Copy link
Member

amareshsm commented May 24, 2022

@mdjermanovic
In new docs, a lot of links seem to be not working. I have listed a few samples.
link - https://new.eslint.org/docs/user-guide/getting-started/

In the below image configuration, command line interface and integration links redirects to 404 page
image

I have created a draft pr #15919. I would like to work on this. Can I find and fix the broken links?

@mdjermanovic
Copy link
Member Author

link - https://new.eslint.org/docs/user-guide/getting-started/

On the current site, this page is https://eslint.org/docs/user-guide/getting-started (without trailing slash). Relative links on the page were set based on the URL without the trailing slash, and the current site redirects https://eslint.org/docs/user-guide/getting-started/ to https://eslint.org/docs/user-guide/getting-started, so it always works well.

On the new site, redirects don't work, and with the trailing slash in https://new.eslint.org/docs/user-guide/getting-started/ relative links on the page are broken.

I have created a draft pr #15919. I would like to work on this. Can I find and fix the broken links?

I think we should decide on the approach first. Some ways to address this might be:

  1. Make redirects work on new.eslint.org/docs. I believe this would be ideal, but not sure if it is possible with Netlify's proxying.
  2. Disable trailing slash redirects on docs-eslint.netlify.app so that it returns 404 on missing/extra trailing slash, if that's possible on Netlify.
  3. Allow all URLs with and without a trailing slash, but use only domain-relative links (start with /) everywhere. It would probably be good to also decide for each page what would be its canonical URL, and generate rel="canonical" link tags and the sitemap accordingly.

@amareshsm
Copy link
Member

In our case can't we use netlify’s Pretty URLs feature by any means? if not then we can consider 3rd option (Allow all URLs with and without a trailing slash)

@nzakas nzakas moved this from Needs Triage to Feedback Needed in Triage Jun 4, 2022
@nzakas
Copy link
Member

nzakas commented Jun 4, 2022

I think the first step here should be to find what is different between the current site setup and the new site setup. We are using the same source files so we should be able to generate the same files to deploy. If we aren’t, it’s probably a misconfiguration in Eleventy.

@mdjermanovic
Copy link
Member Author

I think the first step here should be to find what is different between the current site setup and the new site setup. We are using the same source files so we should be able to generate the same files to deploy. If we aren’t, it’s probably a misconfiguration in Eleventy.

I prepared PR #15967 that should fix this, files will be generated in the same way as on the current site. That should help with using the expected URLs (those where document-relative links on the page work as intended) in the docs index, sitemap and rel="canonical" link tags.

But we'll still have duplicate content and broken links on "wrong" URLs due to the absence of redirections with proxying (#15844 (comment)).

@nzakas
Copy link
Member

nzakas commented Jun 7, 2022

I’ll take a closer look at the proxying issue.

@nzakas
Copy link
Member

nzakas commented Jun 8, 2022

Enabling pretty URLs on the prototype docs site seems to work as-is:
https://docs.eslint.org/rules/no-cond-assign
https://docs.eslint.org/rules/no-cond-assign/

I also enabled it on in-repo docs site, and we don't get 404s, but each URL loads separately:
https://new.eslint.org/docs/rules/no-cond-assign
https://new.eslint.org/docs/rules/no-cond-assign/

It seems like at this point we can probably experiment with URL rewrites on new.eslint.org to solve this problem?

Note: In both pages on new.eslint.org, the canonical URL is set to with a trailing slash because both URLs actually go to the same file.

@mdjermanovic
Copy link
Member Author

I also enabled it on in-repo docs site, and we don't get 404s, but each URL loads separately:
https://new.eslint.org/docs/rules/no-cond-assign
https://new.eslint.org/docs/rules/no-cond-assign/

It seems like at this point we can probably experiment with URL rewrites on new.eslint.org to solve this problem?

Ideally, https://new.eslint.org/docs/rules/no-cond-assign/ would 301 to https://new.eslint.org/docs/rules/no-cond-assign, the same as it currently works on eslint.org.

If that's not possible with proxying on Netlify, I think the next best option would be to return 404. Would disabling Netlify's Pretty URLs option on docs-eslint.netlify.app achieve this?

If neither 301 nor 404 is possible to set up, i.e. if both URLs will always return 200 with the same content, we might need to ensure that all links on all pages have domain-relative URLs so that it doesn't matter whether or not the request URL had a trailing slash.

@nzakas
Copy link
Member

nzakas commented Jun 9, 2022

I don't think we are going to be able to get the exact same behavior as the current site, and I think that's fine. There's no reason to believe the current behavior is any more correct than any other behavior so long as people can find the content they are looking for. Most sites operate with a trailing slash on their URLs, so I'm not too worried about it. As long as the canonical URLs are correct, I think we will be set.

If that's not possible with proxying on Netlify, I think the next best option would be to return 404. Would disabling Netlify's Pretty URLs option on docs-eslint.netlify.app achieve this?

I don't like this approach. We are penalizing the user, who likely arrived here through no fault of their own, by returning a 404.

If neither 301 nor 404 is possible to set up, i.e. if both URLs will always return 200 with the same content, we might need to ensure that all links on all pages have domain-relative URLs so that it doesn't matter whether or not the request URL had a trailing slash.

Can you explain what you mean here? I'm not quite sure I follow.

@nzakas
Copy link
Member

nzakas commented Jun 9, 2022

I dug into this a bit more -- Netlify doesn't allow you to add or remove trailing slashes using redirects.

I'm not sure if this matters much as long as we put in canonical URLs, especially because we will be changing from /docs/* to /docs/latest/*, so we're already changing URLs going forward.

However, we can certainly see if we can get a behavior closer to what we have already by merging #15967 and keeping pretty URLs on. I think that will give us the desired result, but it's hard to know without actually deploying and testing it out.

@mdjermanovic
Copy link
Member Author

If neither 301 nor 404 is possible to set up, i.e. if both URLs will always return 200 with the same content, we might need to ensure that all links on all pages have domain-relative URLs so that it doesn't matter whether or not the request URL had a trailing slash.

Can you explain what you mean here? I'm not quite sure I follow.

If both URLs return the same content, and that content has document-relative links, on one of those two URLs those links will be broken.

nzakas pushed a commit that referenced this issue Jun 10, 2022
…#15967)

* chore: avoid generating subdirectories for each page on new docs site

Refs #15844

* add comments

* refactor pathPrefix

* update component library permalinks

* rewrite URLs

* fix indentation in package.json
@mdjermanovic mdjermanovic changed the title Bug: [new docs site] relative links Bug: [new docs site] relative links on URLs with and without a trailing slash Jun 16, 2022
@mdjermanovic mdjermanovic changed the title Bug: [new docs site] relative links on URLs with and without a trailing slash [new docs site] relative links on URLs with and without a trailing slash Jun 16, 2022
@mdjermanovic
Copy link
Member Author

This is still an issue. For example, all relative links on https://new.eslint.org/docs/latest/user-guide/configuring are broken.

The sidebar link, sitemap, and Canonical URL will all point to https://eslint.org/docs/latest/user-guide/configuring/, which is good, but someone could still land on https://eslint.org/docs/latest/user-guide/configuring by following a link from an external site.

This is a realistic example. Before #13837, https://eslint.org/docs/user-guide/configuring without a trailing slash was the URL for the configuring page, so it's quite possible that this link exists on other sites, and it will now redirect to https://eslint.org/docs/latest/user-guide/configuring.

@nzakas
Copy link
Member

nzakas commented Jun 23, 2022

This is a realistic example. Before #13837, https://eslint.org/docs/user-guide/configuring without a trailing slash was the URL for the configuring page, so it's quite possible that this link exists on other sites, and it will now redirect to https://eslint.org/docs/latest/user-guide/configuring.

I wonder if we can handle this case with specific redirects instead of the generic ones?

Otherwise, I think it’s time we just go through and update the markdown files to have fully qualified URLs. I’m pretty sure we can use the Nunjucks url filter in markdown.

Do you have other ideas?

@mdjermanovic
Copy link
Member Author

I wonder if we can handle this case with specific redirects instead of the generic ones?

Redirects are the best practice for the trailing slash problem, but even if this is feasible they would have to be stored in new.eslint.org repo and basically every URL is problematic so it could be difficult to maintain.

Otherwise, I think it’s time we just go through and update the markdown files to have fully qualified URLs. I’m pretty sure we can use the Nunjucks url filter in markdown.

Given what options we have until Netlify provides some built-in solution, this might be the best choice.

Do you have other ideas?

We could maybe try out <base> tags.

@nzakas
Copy link
Member

nzakas commented Jun 23, 2022

Ooh, <base> seems like a good idea. Let's try that:
#16046

Alternate approach:

        if (!/(?:\/|\.html)$/.test(location.href)) {
            history.replaceState({}, "", location.href + "/");
        }

Triage automation moved this from Feedback Needed to Complete Jun 28, 2022
nzakas added a commit that referenced this issue Jun 28, 2022
* fix: Set base URL on pages so relative URL links work

Fixes #15844

* Use relative base URL
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 26, 2022
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Dec 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly documentation Relates to ESLint's documentation
Projects
Archived in project
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants