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

Simplify no-unclosed-tag rule #340

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ export function parseHtmlNode(p5Node: IP5TagNode, parent: HtmlNode | undefined,

const htmlNodeBase: IHtmlNodeBase = {
tagName: p5Node.tagName.toLowerCase(),
selfClosed: isSelfClosed(p5Node, context),
attributes: [],
location: makeHtmlNodeLocation(p5Node, context),
children: [],
Expand All @@ -69,17 +68,6 @@ export function parseHtmlNode(p5Node: IP5TagNode, parent: HtmlNode | undefined,
return htmlNode;
}

/**
* Returns if this node is self-closed.
* @param p5Node
* @param context
*/
function isSelfClosed(p5Node: IP5TagNode, context: ParseHtmlContext) {
const isEmpty = p5Node.childNodes == null || p5Node.childNodes.length === 0;
const isSelfClosed = getSourceLocation(p5Node)!.startTag.endOffset === getSourceLocation(p5Node)!.endOffset;
return isEmpty && isSelfClosed;
}

/**
* Creates source code location from a p5Node.
* @param p5Node
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ export interface IHtmlNodeBase {
attributes: HtmlNodeAttr[];
parent?: HtmlNode;
children: HtmlNode[];
selfClosed: boolean;
document: HtmlDocument;
}

Expand Down
43 changes: 33 additions & 10 deletions packages/lit-analyzer/src/lib/rules/no-unclosed-tag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,29 @@ import { RuleModule } from "../analyze/types/rule/rule-module.js";
import { isCustomElementTagName } from "../analyze/util/is-valid-name.js";
import { rangeFromHtmlNode } from "../analyze/util/range-util.js";

// List taken from https://html.spec.whatwg.org/multipage/syntax.html#void-elements
// and parse5 list of void elements: https://github.com/inikulin/parse5/blob/86f09edd5a6840ab2269680b0eef2945e78c38fd/packages/parse5/lib/serializer/index.ts#L7-L26
const VOID_ELEMENTS = new Set([
"area",
"base",
"basefont",
"bgsound",
"br",
"col",
"embed",
"frame",
"hr",
"img",
"input",
"keygen",
"link",
"meta",
"param",
"source",
"track",
"wbr"
]);

/**
* This rule validates that all tags are closed properly.
*/
Expand All @@ -11,18 +34,18 @@ const rule: RuleModule = {
priority: "low"
},
visitHtmlNode(htmlNode, context) {
if (!htmlNode.selfClosed && htmlNode.location.endTag == null) {
// Report specifically that a custom element cannot be self closing
// if the user is trying to close a custom element.
const isCustomElement = isCustomElementTagName(htmlNode.tagName);

context.report({
location: rangeFromHtmlNode(htmlNode),
message: `This tag isn't closed.${isCustomElement ? " Custom elements cannot be self closing." : ""}`
});
if (VOID_ELEMENTS.has(htmlNode.tagName.toLowerCase()) || htmlNode.location.endTag != null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we warn on an invalid closing element for a void element? Seems like it'd be good to do so. For example, this should be a warning on the </input>

<input><div></div></input>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a good idea but maybe something we can add incrementally to the no-unclosed-tag diagnostic.

I'll make an issue.

return;
}

return;
// Report specifically that a custom element cannot be self closing
// if the user is trying to close a custom element.
const isCustomElement = isCustomElementTagName(htmlNode.tagName);

context.report({
location: rangeFromHtmlNode(htmlNode),
message: `This tag isn't closed.${isCustomElement ? " Custom elements cannot be self closing." : ""}`
});
}
};

Expand Down
36 changes: 35 additions & 1 deletion packages/lit-analyzer/src/test/rules/no-unclosed-tag.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,41 @@ tsTest("Report unclosed tags", t => {
hasDiagnostic(t, diagnostics, "no-unclosed-tag");
});

Copy link
Collaborator

Choose a reason for hiding this comment

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

In an ideal world, we might want to allow:

<p>One
<p>Two

Or just:

<p>Hi

And we'd want to put warnings on dangling closing tags that don't match up to any opening tags.

All of those may be somewhat tricky to do with how parse5 represents the tree though, requiring us to notice spans in the original text that aren't represented in the parsed tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are very tricky, and another good case to add to an issue for followup.

Philosophically though, I wonder if the lit-analyzer should avoid HTML diagnostics and leave that to an HTML specific tool. It may even be worth scoping the no-unclosed-tag to only check custom element tags.

tsTest("Don't report self closed tags", t => {
tsTest("Don't report void elements", t => {
const { diagnostics } = getDiagnostics("html`<img>`", { rules: { "no-unclosed-tag": true } });
hasNoDiagnostics(t, diagnostics);
});

tsTest("Don't report void elements with self closing syntax", t => {
const { diagnostics } = getDiagnostics("html`<img />`", { rules: { "no-unclosed-tag": true } });
hasNoDiagnostics(t, diagnostics);
});

// The `<p>` tag will be closed automatically if immediately followed by a lot of other elements,
// including `<div>`.
// Ref: https://html.spec.whatwg.org/multipage/grouping-content.html#the-p-element
tsTest("Report unclosed 'p' tag that was implicitly closed via tag omission", t => {
const { diagnostics } = getDiagnostics("html`<p><div></div></p>`", { rules: { "no-unclosed-tag": true } });
hasDiagnostic(t, diagnostics, "no-unclosed-tag");
});

tsTest("Report unclosed 'p' tag that is implicitly closed via tag omission containing text content", t => {
const { diagnostics } = getDiagnostics("html`<p>Unclosed Content<div></div></p>`", { rules: { "no-unclosed-tag": true } });
hasDiagnostic(t, diagnostics, "no-unclosed-tag");
});

// Self-closing tags do not exist in HTML. They are only valid in SVG and MathML.
tsTest("Report non-void element using self closing syntax", t => {
const { diagnostics } = getDiagnostics("html`<p /><div></div>`", { rules: { "no-unclosed-tag": true } });
hasDiagnostic(t, diagnostics, "no-unclosed-tag");
});

tsTest("Report self closing 'p' tag containing text content", t => {
const { diagnostics } = getDiagnostics("html`<p />Unclosed Content<div></div>`", { rules: { "no-unclosed-tag": true } });
hasDiagnostic(t, diagnostics, "no-unclosed-tag");
});

tsTest("Don't report explicit closing 'p' tag containing text content", t => {
const { diagnostics } = getDiagnostics("html`<p>Unclosed Content</p><div></div>`", { rules: { "no-unclosed-tag": true } });
hasNoDiagnostics(t, diagnostics);
});
Loading