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

Docs: fix multiple broken links #14743

Closed
wants to merge 1 commit into from

Conversation

chenxsan
Copy link
Contributor

@chenxsan chenxsan commented Jun 24, 2021

Prerequisites checklist

What is the purpose of this pull request? (put an "X" next to an item)

[x] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofixing to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:

What changes did you make? (Give an overview)

Fix multiple broken links.

The markdown plugin defined here https://github.com/eslint/website/blob/master/_11ty/plugins/markdown-plugins.js#L40 would replace -- with -, take https://eslint.org/docs/user-guide/command-line-interface#-resolve-plugins-relative-to for example:

CleanShot 2021-06-24 at 22 27 50@2x

Namely, header anchors will contain only one - instead of two. Hence it's wrong to use -- when linking to those header anchors.

Is there anything you'd like reviewers to focus on?

Nothing.

@eslint-github-bot eslint-github-bot bot added the triage An ESLint team member will look at this issue soon label Jun 24, 2021
Copy link
Contributor

@snitin315 snitin315 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!

@snitin315 snitin315 added accepted There is consensus among the team that this change meets the criteria for inclusion documentation Relates to ESLint's documentation and removed triage An ESLint team member will look at this issue soon labels Jun 24, 2021
@mdjermanovic
Copy link
Member

The markdown plugin defined here https://github.com/eslint/website/blob/master/_11ty/plugins/markdown-plugins.js#L40 would replace -- with -, take https://eslint.org/docs/user-guide/command-line-interface#-resolve-plugins-relative-to for example:

GitHub expects -- in the link:

https://github.com/eslint/eslint/blob/master/docs/user-guide/command-line-interface.md#--resolve-plugins-relative-to

Should we rather change how the ids are generated on the website? While links on the website don't necessarily have to match how GitHub works, it would be easier for development and testing.

@chenxsan
Copy link
Contributor Author

chenxsan commented Jun 25, 2021

The code was introduced 11 months ago in eslint/archive-website#755. I'm afraid there might be many links with single - from external websites now. If we're going to change how ids are generated, we're likely to break all those links.

For instance, here's one I just found https://stackoverflow.com/a/65050590/1077489. You can see a link of https://eslint.org/docs/user-guide/command-line-interface#-rule, it will be broken once we generate header ids with double -.

But I agree it's better to align the website with files on Github. So maybe setting up some redirections on the website if we decide to update the rule? E.g., redirect https://eslint.org/docs/user-guide/command-line-interface#-rule to https://eslint.org/docs/user-guide/command-line-interface#--rule. But I'm not sure if it's feasible as there could be many links to include.

@mdjermanovic
Copy link
Member

The code was introduced 11 months ago in eslint/website#755. I'm afraid there might be many links with single - from external websites now. If we're going to change how ids are generated, we're likely to break all those links.

For instance, here's one I just found https://stackoverflow.com/a/65050590/1077489. You can see a link of https://eslint.org/docs/user-guide/command-line-interface#-rule, it will be broken once we generate header ids with double -.

I personally think that's okay. It's probably only one page and the links will still open the right page, just without scrolling to the section.

But I agree it's better to align the website with files on Github. So maybe setting up some redirections on the website if we decide to update the rule? E.g., redirect https://eslint.org/docs/user-guide/command-line-interface#-rule to https://eslint.org/docs/user-guide/command-line-interface#--rule. But I'm not sure if it's feasible as there could be many links to include.

I don't think it's possible to do such server redirections as # fragment isn't sent to the server. We could only do this with a JS script in browser, but it seems unnecessary and difficult to maintain.

@chenxsan
Copy link
Contributor Author

Cool, then I'm going to close this.

@chenxsan chenxsan closed this Jun 25, 2021
@chenxsan chenxsan deleted the bugfix/fix-double-dash branch June 25, 2021 13:44
@chenxsan
Copy link
Contributor Author

chenxsan commented Jun 25, 2021

However changing how the ids are generated on the website might cause other problems:

md = md.use(require("markdown-it-anchor"), {
        slugify(text) {

            // need to replace all the characters GitHub replaces
            return slug(text.replace(/[<>()[\]{}]/gu, ""))

                // non-ASCII characters are a pain to fix
                // first replace them all with dashes
                .replace(/[^\u{00}-\u{FF}]/gu, "-")

                // then replace all -- with -
                .replace(/-+/gu, "-");
        },
        uniqueSlugStartIndex: 1
    });

Because the markdown plugin is replacing non-ASCII characters with dashes first.

Take https://eslint.org/docs/user-guide/configuring/ignoring-code#the-eslintignore-file for example, the current id is the-eslintignore-file. Once .replace(/-+/gu, "-"); gets removed, that id will be the--eslintignore-file (I guess?). Then we would introduce another broken links on the website.

@mdjermanovic
Copy link
Member

I'm not sure why are we doing that in two steps, I think that [^\u{00}-\u{FF}] characters can be just removed. We could try out something like this, and then check which files in the final html output have changed:

- .replace(/[^\u{00}-\u{FF}]/gu, "-")
- .replace(/-+/gu, "-");
+ .replace(/[^\u{00}-\u{FF}]/gu, "");  

Take https://eslint.org/docs/user-guide/configuring/ignoring-code#the-eslintignore-file for example, the current id is the-eslintignore-file. Once .replace(/-+/gu, "-"); gets removed, that id will be the--eslintignore-file (I guess?). Then we would introduce another broken links on the website.

I didn't understand this example. The input text is "The .eslintignore File"? If that's the case, slug() (github-slugger) will convert it to the-eslintignore-file.

@chenxsan
Copy link
Contributor Author

Oops, you're right, that example is a bad one, I should run it with code first :)

I agree with your suggestion here. It's a nice method to evaluate the potential consequence of that change.

@chenxsan
Copy link
Contributor Author

chenxsan commented Jun 26, 2021

@mdjermanovic I've applied your suggestion locally and run the build command to compare the two built versions, there're about 123 files changed - excluding the markdown-plugins.js file. Most are located in docs/rules directory.

E.g.,

CleanShot 2021-06-26 at 08 52 54@2x

Generally I think it's ok to make those changes. But you might want to assess it yourself.

Btw, once applied, URI fragments like #-fix shall be updated to #--fix as well.

@mdjermanovic
Copy link
Member

mdjermanovic commented Jun 26, 2021

123 sounds good, docs/7.0.0/ and similar directories are leftovers from prereleases, so this should be like ~ 15 actual files.

By your list, it looks like half of the changes will be in <h1> (spaces converted to - around characters which are removed like *, {} etc. in titles of rules), which shouldn't matter for links, so aside from command-line-interface I'd expect only 5-6 significant changes.

Changes like lines-around-directive match with GitHub:

https://github.com/eslint/eslint/blob/master/docs/rules/lines-around-directive.md#before--after

I think we should do this.

(I couldn't figure out - what will be changed in no-inner-declarations?)

@chenxsan
Copy link
Contributor Author

There're two whitespaces between instead of one:

CleanShot 2021-06-27 at 10 13 25@2x

I think we can remove the superfluous whitespace once the markdown-it plugin gets updated, and that would keep the id generated same as before.

@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Dec 23, 2021
@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 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion documentation Relates to ESLint's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants