feat: add eslint plugin no-html-links #8156

JohnVicke · 2022-10-01T17:22:04Z

Pre-flight checklist

I have read the Contributing Guidelines on pull requests.
If this is a code change: I have written unit tests and/or added dogfooding pages to fully verify the new behavior.

Motivation

Described in: #6472

Test Plan

Jest unit tests are included.

Output of `yarn test no-html-plugins`

PASS packages/eslint-plugin/src/rules/__tests__/no-html-links.test.ts
  prefer-docusaurus-link
    valid
      ✓ <Link to="/test">test</Link> (239 ms)
      ✓ <Link to="https://twitter.com/docusaurus">Twitter</Link> (2 ms)
    invalid
      ✓ <a href="/test">test</a> (3 ms)
      ✓ <a href="https://twitter.com/docusaurus" target="_blank">test</a> (2 ms)
      ✓ <a href="https://twitter.com/docusaurus" target="_blank" rel="noopener noreferrer">test</a> (1 ms)

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   0 total
Time:        0.67 s, estimated 1 s
Ran all test suites matching /no-html-links/i.

Test links

Docs preview: https://deploy-preview-8156--docusaurus-2.netlify.app/docs/api/misc/@docusaurus/eslint-plugin/no-html-links

Comments

The solution is fairly simple, I played around with when the message should be returned to the user (e.g. a tag has no attributes). However, if we want to enforce docusaurus links all the time this feels more accurate.

netlify · 2022-10-01T17:27:00Z

✅ [V2]

Name	Link
🔨 Latest commit	`741e6ef`
🔍 Latest deploy log	https://app.netlify.com/sites/docusaurus-2/deploys/639a04498d4ae30009f80f2e
😎 Deploy Preview	https://deploy-preview-8156--docusaurus-2.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

github-actions · 2022-10-01T17:27:38Z

⚡️ Lighthouse report for the deploy preview of this PR

URL	Performance	Accessibility	Best Practices	SEO	PWA	Report
/	🔴 36	🟢 97	🟢 100	🟢 100	🟢 90	Report
/docs/installation	🟠 73	🟢 100	🟢 100	🟢 100	🟢 90	Report

slorber

Thanks, LGTM 👍

Wonder if we want to be that strict and never allow any absolute URL by default, so maybe adding an option can be convenient?

We mostly want to prevent bad relative links IMHO

Any opinion @Josh-Cena ?

slorber · 2022-10-05T15:48:27Z

website/src/components/TweetQuote/index.tsx

      </blockquote>
      <figcaption>
-        <a href={profileUrl} target="_blank" rel="noreferrer nofollow">


<Link> doesn't automatically apply nofollow.

Maybe use <Link rel="nofollow">. Open question: should the rel be merged into existing ones ("noreferrer noopener") or override?

I assume merging is fine because we generally don't want to remove "noopener noreferrer" from external links (and could use <a> as an escape hatch)

website/docs/api/misc/eslint-plugin/README.md

website/docs/api/misc/eslint-plugin/no-html-links.md

packages/eslint-plugin/src/rules/no-html-links.ts

slorber · 2022-10-05T15:57:30Z

packages/docusaurus-theme-classic/src/theme/TOCItems/Tree.tsx

-          <a
-            href={`#${heading.id}`}
+          <Link
+            to={`#${heading.id}`}


wonder if we could also allow <a> for hardcoded hash links?

(not all links can be evaluated but hardcoded ones could? Maybe eslint can know it starts with a hash? 🤷‍♂️)

It's not undoable. Even for template literals, you just need to check quasis[0][0] === "#".

Good point!

Lets say that we add this to the plugin, should it still warn (encourage) the user to use <Link> instead of the a tag?

I guess we can just ignore hashes. This is a bonus, we can merge this PR without this feature if complicated to implement.

packages/eslint-plugin/src/rules/no-html-links.ts

Josh-Cena · 2022-10-05T16:04:01Z

Wonder if we want to be that strict and never allow any absolute URL by default, so maybe adding an option can be convenient?

An option like ignoreFullyResolved would be my go-to, yes. It would be tricky to check template literal values, though. Recommending always using <Link> is not that bad and may allow us to do other useful things in the future.

slorber · 2022-10-05T16:59:45Z

ignoreFullyResolved seems fine, even if it only allows hardcoded href props that can already be convenient.

Not sure "fully resolved" is very common, where does it come from?

Recommending always using is not that bad and may allow us to do other useful things in the future.

Agree, but maybe we'll need to improve it for external links then , because for example not merging rel (current behavior) is a bit annoying

JohnVicke · 2022-10-05T18:57:08Z

ignoreFullyResolved seems fine, even if it only allows hardcoded href props that can already be convenient.

Thanks for the thorough feedback, will add some changes tomorrow 🙇 . ignoreFullyResolved got me thinking of another use-case we might want to consider. Should we add an option to ignore mailto or let them pass as default behaviour?

Josh-Cena · 2022-10-05T19:18:07Z

I'd say, anything that starts with a protocol should be ignored.

slorber · 2022-10-07T14:56:17Z

I'd say, anything that starts with a protocol should be ignored.

agree

At the same time is fullyResolved the term to use for an URL that starts with a protocol? 🤷‍♂️ I don't know.

It looks like the concept of "fully qualified" exist for domain names (https://www.rfc-editor.org/rfc/rfc6454 https://en.wikipedia.org/wiki/Fully_qualified_domain_name) but can't really find any official ref of "fully qualified URL" in any spec).

I don't know where "fully resolved" comes from but it doesn't seem like an existing concept either.

I'm more used to see the term "fully qualified" and some sources seem agree despite being web authorities (https://gateway.force.com/DPGCustHelp/s/article/Fully-qualified-URL)

Josh-Cena · 2022-10-07T14:57:29Z

I was thinking about "fully resolved paths". I don't know where that term crosses my mind either.

slorber

LGTM, good enough to merge

Just wondering if "ignoreFullyResolved" is the best term we can use to represent the option, any opinion before merging?

slorber · 2022-10-07T15:02:23Z

website/docs/api/misc/eslint-plugin/no-html-links.md

+
+| Option | Type | Default | Description |
+| --- | --- | --- | --- |
+| `ignoreFullyResolved` | `boolean` | `false` | Set to true will not report any `<a>` tags with absolute URLs. |


if "fully resolved" === "absolute url" then maybe we'd want to be consistent?

https://www.rfc-editor.org/rfc/rfc3986#section-4.3

Maybe we just want "ignoreAbsoluteUrls" instead, it would be easier for users to understand?

This is just wording, but it remains important.

Now we can still do a breaking change later if we change our mind, not a huge deal.

website/docs/api/misc/eslint-plugin/no-html-links.md

Josh-Cena · 2022-10-07T15:04:23Z

I want to review this before merging. I think I can find some time this weekend

# Conflicts: # packages/docusaurus-theme-classic/src/theme/SkipToContent/index.tsx

slorber · 2022-10-21T12:45:22Z

conflict resolved

@Josh-Cena do you still want to review it?

slorber · 2022-10-26T18:11:35Z

@Josh-Cena ?

Josh-Cena

Extremely sorry for missing this @JohnVicke. The implementation looks good although I'd like to work on enhancements after this is merged. Just requesting a bit more tests (as ESLint plugin maintainers always do...) I could also do this myself this week so it gets merged ASAP

Josh-Cena · 2022-10-26T18:27:01Z

packages/eslint-plugin/src/rules/no-html-links.ts

+            (attr): attr is TSESTree.JSXAttribute => attr.type === 'JSXAttribute' && attr.name.name === 'href',
+          );
+
+          if (hrefAttr?.value?.type === 'Literal') {


Could you add tests for when this is not a literal? Especially template literals. In the future I definitely want this rule to recognize some common interpolation patterns (e.g. if the template starts with / then it's definitely not fullyResolved), so it's better if we can have some tests to verify how the behaviors change.

website/docs/api/misc/eslint-plugin/no-html-links.md

slorber · 2022-12-09T17:40:56Z

@Josh-Cena let me know if you want to do something about this PR 🤪 otherwise I'll apply some suggestions, do a few minor changes and merge it

Josh-Cena · 2022-12-09T19:57:47Z

Mostly LGTM. I wonder if we should add an option to ignore fully dynamic and not statically analyzable href props, but that could be an extension because using Link can't harm in any case.

Co-authored-by: Joshua Chen <[email protected]>

slorber · 2022-12-14T17:06:40Z

Look good enough to merge

Added some extra code to handle hardcoded/static template literal strings too href={"https://xyz.com"} as it wasn't the case.

For more dynamic cases where static analysis is possible, that can be handled in a follow-up PR

Was wondering if it's possible to pre-evaluate AST nodes with TSESTree @Josh-Cena @JoshuaKGoldberg

Apparently, Babel is able to do that successfully:

Feel free to complete the missing cases if you know how to ;)

Viktor Malmedal added 3 commits October 1, 2022 18:55

feat: add eslint plugin no-html-links

2c8ff51

docs: add docs for no-html-links

0ec09b0

fix: eslint no-html-links errors

6e80680

JohnVicke requested review from slorber, lex111 and Josh-Cena as code owners October 1, 2022 17:22

facebook-github-bot added the CLA Signed Signed Facebook CLA label Oct 1, 2022

slorber requested changes Oct 5, 2022

View reviewed changes

Viktor Malmedal added 5 commits October 6, 2022 08:37

add rel nofollow to link

617153a

fix formatting of a tag in md

695a49a

add tests for ignoreFullyResolved

7434b77

add option ignoreFullyIgnore

f985e86

docs: update no-html-links

237d99e

slorber added the pr: new feature This PR adds a new API or behavior. label Oct 7, 2022

slorber approved these changes Oct 7, 2022

View reviewed changes

Merge branch 'main' into eslint-no-html-links

37e3e0b

# Conflicts: # packages/docusaurus-theme-classic/src/theme/SkipToContent/index.tsx

Refactor

add5058

Josh-Cena reviewed Oct 26, 2022

View reviewed changes

slorber and others added 6 commits December 14, 2022 16:35

Merge branch 'main' into eslint-no-html-links

f7f6828

prettier

c3a885b

Update website/docs/api/misc/eslint-plugin/no-html-links.md

d50a497

Co-authored-by: Joshua Chen <[email protected]>

add link to <Link> in doc

72581f5

clarify ignoreFullyResolved

41ef7a5

add more tests + simple template literal handling

bf39059

empty

741e6ef

slorber added the to backport This PR is planned to be backported to a stable version of Docusaurus label Dec 14, 2022

slorber merged commit 4a44877 into facebook:main Dec 14, 2022

slorber removed the to backport This PR is planned to be backported to a stable version of Docusaurus label Jan 26, 2023

This was referenced Oct 19, 2023

chore: v3.0.0-rc.0 release #9418

Merged

chore: v3.0.0-rc.1 release #9453

Merged

Abuchtela mentioned this pull request May 20, 2024

[Snyk] Upgrade @docusaurus/preset-classic from 3.0.0 to 3.2.1 Abuchtela/aptos-core#42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add eslint plugin no-html-links #8156

feat: add eslint plugin no-html-links #8156

JohnVicke commented Oct 1, 2022 •

edited

Loading

netlify bot commented Oct 1, 2022 •

edited

Loading

github-actions bot commented Oct 1, 2022 •

edited

Loading

slorber left a comment

slorber Oct 5, 2022

slorber Oct 5, 2022

Josh-Cena Oct 5, 2022

JohnVicke Oct 5, 2022 •

edited

Loading

slorber Oct 7, 2022

Josh-Cena commented Oct 5, 2022

slorber commented Oct 5, 2022 •

edited

Loading

JohnVicke commented Oct 5, 2022 •

edited

Loading

Josh-Cena commented Oct 5, 2022

slorber commented Oct 7, 2022

Josh-Cena commented Oct 7, 2022

slorber left a comment

slorber Oct 7, 2022

Josh-Cena commented Oct 7, 2022

slorber commented Oct 21, 2022

slorber commented Oct 26, 2022

Josh-Cena left a comment •

edited

Loading

Josh-Cena Oct 26, 2022

slorber commented Dec 9, 2022

Josh-Cena commented Dec 9, 2022

slorber commented Dec 14, 2022

feat: add eslint plugin no-html-links #8156

feat: add eslint plugin no-html-links #8156

Conversation

JohnVicke commented Oct 1, 2022 • edited Loading

Pre-flight checklist

Motivation

Test Plan

Test links

Comments

netlify bot commented Oct 1, 2022 • edited Loading

✅ [V2]

github-actions bot commented Oct 1, 2022 • edited Loading

⚡️ Lighthouse report for the deploy preview of this PR

slorber left a comment

Choose a reason for hiding this comment

slorber Oct 5, 2022

Choose a reason for hiding this comment

slorber Oct 5, 2022

Choose a reason for hiding this comment

Josh-Cena Oct 5, 2022

Choose a reason for hiding this comment

JohnVicke Oct 5, 2022 • edited Loading

Choose a reason for hiding this comment

slorber Oct 7, 2022

Choose a reason for hiding this comment

Josh-Cena commented Oct 5, 2022

slorber commented Oct 5, 2022 • edited Loading

JohnVicke commented Oct 5, 2022 • edited Loading

Josh-Cena commented Oct 5, 2022

slorber commented Oct 7, 2022

Josh-Cena commented Oct 7, 2022

slorber left a comment

Choose a reason for hiding this comment

slorber Oct 7, 2022

Choose a reason for hiding this comment

Josh-Cena commented Oct 7, 2022

slorber commented Oct 21, 2022

slorber commented Oct 26, 2022

Josh-Cena left a comment • edited Loading

Choose a reason for hiding this comment

Josh-Cena Oct 26, 2022

Choose a reason for hiding this comment

slorber commented Dec 9, 2022

Josh-Cena commented Dec 9, 2022

slorber commented Dec 14, 2022

JohnVicke commented Oct 1, 2022 •

edited

Loading

netlify bot commented Oct 1, 2022 •

edited

Loading

github-actions bot commented Oct 1, 2022 •

edited

Loading

JohnVicke Oct 5, 2022 •

edited

Loading

slorber commented Oct 5, 2022 •

edited

Loading

JohnVicke commented Oct 5, 2022 •

edited

Loading

Josh-Cena left a comment •

edited

Loading