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

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

Merged
merged 17 commits into from
Dec 14, 2022
Merged
1 change: 1 addition & 0 deletions .eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -374,6 +374,7 @@ module.exports = {
// locals must be justified with a disable comment.
'@typescript-eslint/no-unused-vars': [ERROR, {ignoreRestSiblings: true}],
'@typescript-eslint/prefer-optional-chain': ERROR,
'@docusaurus/no-html-links': ERROR,
'@docusaurus/no-untranslated-text': [
WARNING,
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,22 +8,19 @@
import React from 'react';
import Translate from '@docusaurus/Translate';
import {ThemeClassNames} from '@docusaurus/theme-common';
import Link from '@docusaurus/Link';
import IconEdit from '@theme/Icon/Edit';
import type {Props} from '@theme/EditThisPage';

export default function EditThisPage({editUrl}: Props): JSX.Element {
return (
<a
href={editUrl}
target="_blank"
rel="noreferrer noopener"
className={ThemeClassNames.common.editThisPage}>
<Link to={editUrl} className={ThemeClassNames.common.editThisPage}>
<IconEdit />
<Translate
id="theme.common.editThisPage"
description="The link label to edit the current page">
Edit this page
</Translate>
</a>
</Link>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import React from 'react';
import clsx from 'clsx';
import {translate} from '@docusaurus/Translate';
import {useThemeConfig} from '@docusaurus/theme-common';
import Link from '@docusaurus/Link';
import type {Props} from '@theme/Heading';

import styles from './styles.module.css';
Expand All @@ -34,16 +35,16 @@ export default function Heading({as: As, id, ...props}: Props): JSX.Element {
)}
id={id}>
{props.children}
<a
<Link
className="hash-link"
href={`#${id}`}
to={`#${id}`}
title={translate({
id: 'theme.common.headingLinkTitle',
message: 'Direct link to heading',
description: 'Title for link to heading',
})}>
&#8203;
</a>
</Link>
</As>
);
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

import React from 'react';
import {SkipToContentLink} from '@docusaurus/theme-common';

import styles from './styles.module.css';

export default function SkipToContent(): JSX.Element {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/

import React from 'react';
import Link from '@docusaurus/Link';
import type {Props} from '@theme/TOCItems/Tree';

// Recursive component rendering the toc tree
Expand All @@ -22,12 +23,10 @@ function TOCItemTree({
<ul className={isChild ? undefined : className}>
{toc.map((heading) => (
<li key={heading.id}>
{/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}
<a
href={`#${heading.id}`}
<Link
to={`#${heading.id}`}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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? 🤷‍♂️)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@JohnVicke JohnVicke Oct 5, 2022

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

className={linkClassName ?? undefined}
// Developer provided the HTML, so assume it's safe.
// eslint-disable-next-line react/no-danger
dangerouslySetInnerHTML={{__html: heading.value}}
/>
<TOCItemTree
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ export function SkipToContentLink(props: SkipToContentLinkProps): JSX.Element {
ref={containerRef}
role="region"
aria-label={DefaultSkipToContentLabel}>
{/* eslint-disable-next-line @docusaurus/no-html-links */}
<a
{...props}
// Note this is a fallback href in case JS is disabled
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,8 @@ function SearchPageContent(): JSX.Element {
'text--right',
styles.searchLogoColumn,
)}>
<a
target="_blank"
rel="noopener noreferrer"
href="https://www.algolia.com/"
<Link
to="https://www.algolia.com/"
aria-label={translate({
id: 'theme.SearchPage.algoliaLabel',
message: 'Search by Algolia',
Expand All @@ -451,7 +449,7 @@ function SearchPageContent(): JSX.Element {
/>
</g>
</svg>
</a>
</Link>
</div>
</div>

Expand Down
2 changes: 1 addition & 1 deletion packages/docusaurus/src/client/exports/Link.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ function Link(
}

return isRegularHtmlLink ? (
// eslint-disable-next-line jsx-a11y/anchor-has-content
// eslint-disable-next-line jsx-a11y/anchor-has-content, @docusaurus/no-html-links
<a
ref={innerRef}
href={targetLink}
Expand Down
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,15 @@ export = {
plugins: ['@docusaurus'],
rules: {
'@docusaurus/string-literal-i18n-messages': 'error',
'@docusaurus/no-html-links': 'warn',
},
},
all: {
plugins: ['@docusaurus'],
rules: {
'@docusaurus/string-literal-i18n-messages': 'error',
'@docusaurus/no-untranslated-text': 'warn',
'@docusaurus/no-html-links': 'warn',
},
},
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import rule from '../no-html-links';
import {RuleTester} from './testUtils';

const errorsJSX = [{messageId: 'link'}] as const;

const ruleTester = new RuleTester({
parser: '@typescript-eslint/parser',
parserOptions: {
ecmaFeatures: {
jsx: true,
},
},
});

ruleTester.run('prefer-docusaurus-link', rule, {
valid: [
{
code: '<Link to="/test">test</Link>',
},
{
code: '<Link to="https://twitter.com/docusaurus">Twitter</Link>',
},
{
code: '<a href="https://twitter.com/docusaurus">Twitter</a>',
options: [{ignoreFullyResolved: true}],
},
{
code: '<a href={`https://twitter.com/docusaurus`}>Twitter</a>',
options: [{ignoreFullyResolved: true}],
},
{
code: '<a href="mailto:[email protected]">Contact</a> ',
options: [{ignoreFullyResolved: true}],
},
{
code: '<a href="tel:123456789">Call</a>',
options: [{ignoreFullyResolved: true}],
},
],
invalid: [
{
code: '<a href="/test">test</a>',
errors: errorsJSX,
},
{
code: '<a href="https://twitter.com/docusaurus" target="_blank">test</a>',
errors: errorsJSX,
},
{
code: '<a href="https://twitter.com/docusaurus" target="_blank" rel="noopener noreferrer">test</a>',
errors: errorsJSX,
},
{
code: '<a href="mailto:[email protected]">Contact</a> ',
errors: errorsJSX,
},
{
code: '<a href="tel:123456789">Call</a>',
errors: errorsJSX,
},
{
code: '<a href={``}>Twitter</a>',
errors: errorsJSX,
},
{
code: '<a href={`https://www.twitter.com/docusaurus`}>Twitter</a>',
errors: errorsJSX,
},
{
code: '<a href="www.twitter.com/docusaurus">Twitter</a>',
options: [{ignoreFullyResolved: true}],
errors: errorsJSX,
},
{
// TODO we might want to make this test pass
// Can template literals be statically pre-evaluated? (Babel can do it)
// eslint-disable-next-line no-template-curly-in-string
code: '<a href={`https://twitter.com/${"docu" + "saurus"} ${"rex"}`}>Twitter</a>',
options: [{ignoreFullyResolved: true}],
errors: errorsJSX,
},
],
});
2 changes: 2 additions & 0 deletions packages/eslint-plugin/src/rules/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@
* LICENSE file in the root directory of this source tree.
*/

import noHtmlLinks from './no-html-links';
import noUntranslatedText from './no-untranslated-text';
import stringLiteralI18nMessages from './string-literal-i18n-messages';

export default {
'no-untranslated-text': noUntranslatedText,
'string-literal-i18n-messages': stringLiteralI18nMessages,
'no-html-links': noHtmlLinks,
};
103 changes: 103 additions & 0 deletions packages/eslint-plugin/src/rules/no-html-links.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
/**
* Copyright (c) Facebook, Inc. and its affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import {createRule} from '../util';
import type {TSESTree} from '@typescript-eslint/types/dist/ts-estree';

const docsUrl = 'https://docusaurus.io/docs/docusaurus-core#link';

type Options = [
{
ignoreFullyResolved: boolean;
},
];

type MessageIds = 'link';
slorber marked this conversation as resolved.
Show resolved Hide resolved

function isFullyResolvedUrl(urlString: string): boolean {
try {
// href gets coerced to a string when it gets rendered anyway
const url = new URL(String(urlString));
if (url.protocol) {
return true;
}
} catch (e) {}
return false;
}

export default createRule<Options, MessageIds>({
slorber marked this conversation as resolved.
Show resolved Hide resolved
name: 'no-html-links',
meta: {
type: 'problem',
docs: {
description: 'enforce using Docusaurus Link component instead of <a> tag',
recommended: false,
},
schema: [
{
type: 'object',
properties: {
ignoreFullyResolved: {
type: 'boolean',
},
},
additionalProperties: false,
},
],
messages: {
link: `Do not use an \`<a>\` element to navigate. Use the \`<Link />\` component from \`@docusaurus/Link\` instead. See: ${docsUrl}`,
},
},
defaultOptions: [
{
ignoreFullyResolved: false,
},
],

create(context, [options]) {
const {ignoreFullyResolved} = options;

return {
JSXOpeningElement(node) {
if ((node.name as TSESTree.JSXIdentifier).name !== 'a') {
return;
}

if (ignoreFullyResolved) {
const hrefAttr = node.attributes.find(
(attr): attr is TSESTree.JSXAttribute =>
attr.type === 'JSXAttribute' && attr.name.name === 'href',
);

if (hrefAttr?.value?.type === 'Literal') {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

if (isFullyResolvedUrl(String(hrefAttr.value.value))) {
return;
}
}
if (hrefAttr?.value?.type === 'JSXExpressionContainer') {
const container: TSESTree.JSXExpressionContainer = hrefAttr.value;
const {expression} = container;
if (expression.type === 'TemplateLiteral') {
// Simple static string template literals
if (
expression.expressions.length === 0 &&
expression.quasis.length === 1 &&
expression.quasis[0]?.type === 'TemplateElement' &&
isFullyResolvedUrl(String(expression.quasis[0].value.raw))
) {
return;
}
// TODO add more complex TemplateLiteral cases here
}
}
}

context.report({node, messageId: 'link'});
},
};
},
});
21 changes: 5 additions & 16 deletions website/_dogfooding/_pages tests/hydration-tests.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,28 +6,17 @@
*/

import React from 'react';
import Link from '@docusaurus/Link';
import Layout from '@theme/Layout';

// Repro for hydration issue https://github.com/facebook/docusaurus/issues/5617
function BuggyText() {
return (
<span>
Built using the{' '}
<a href="https://www.electronjs.org/" target="_blank" rel="noreferrer">
Electron
</a>{' '}
, based on{' '}
<a href="https://www.chromium.org/" target="_blank" rel="noreferrer">
Chromium
</a>
, and written using{' '}
<a
href="https://www.typescriptlang.org/"
target="_blank"
rel="noreferrer">
TypeScript
</a>{' '}
, Xplorer promises you an unprecedented experience.
Built using the <Link to="https://www.electronjs.org/">Electron</Link> ,
based on <Link to="https://www.chromium.org/">Chromium</Link>, and written
using <Link to="https://www.typescriptlang.org/">TypeScript</Link> ,
Xplorer promises you an unprecedented experience.
</span>
);
}
Expand Down
1 change: 1 addition & 0 deletions website/docs/api/misc/eslint-plugin/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ For more fine-grained control, you can also enable the plugin manually and confi
| --- | --- | --- |
| [`@docusaurus/no-untranslated-text`](./no-untranslated-text.md) | Enforce text labels in JSX to be wrapped by translate calls | |
| [`@docusaurus/string-literal-i18n-messages`](./string-literal-i18n-messages.md) | Enforce translate APIs to be called on plain text labels | ✅ |
| [`@docusaurus/no-html-links`](./no-html-links.md) | Ensures @docusaurus/Link is used instead of `<a>` tags | ✅ |

✅ = recommended

Expand Down
Loading