build: tslint rule to enforce html tags escaping in comments #8200

julianobrasil · 2017-11-02T18:37:26Z

Refactoring of #5825 logic

This rule will look for:

1 - Any HTML open/close tag delimiter (< or >) outside backtick pairs (``)

/** 
 * The using of <svg> is very good because...
 */

/** 
 * Look for every occurrency of `i <= 18...
 */

2 - Any "opening" backtick without its "closing" match:

/** 
 * The using of `<svg> is very good because...
 */

/** 
 * We recommend using backticks (`) to escape...
 */

And it will allow any html provided it's between backtick matching pairs:

/** 
 * The using of `    <div><br></div> is very` good because...
 */

willshowell

Thanks for reviving it!

willshowell · 2017-11-02T19:08:36Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+  visitSourceFile(sourceFile) {
+    utils.forEachComment(sourceFile, (fullText, commentRange) => {
+
+      let isEscapedHtmlTag = true;


htmlIsEscaped?

willshowell · 2017-11-02T19:10:52Z

tslint.json

+      ],
+      [
+        "xdescribe"
+      ]


I think the original formatting was fine. Just add no-unescaped-html-tag without any of the other changes.

Oh, that was and accidental shift+alt+f in Visual Studio Code.

willshowell · 2017-11-02T19:19:20Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+  }
+}
+
+exports.Rule = Rule;


Another way to write this rule that might be cleaner would be,

utils.forEachComment(sourceFile, (fullText, commentRange) => { const htmlIsEscaped = parseForHtml(fullText); if (commentRange.kind === ... && !htmlIsEscaped) { this.addFailureAt(...); } } /** Gets whether the comment's HTML, if any, is properly escaped */ parseForHtml(fullText) { // parsing done here }

I think that way you wouldn't have to worry so much about carrying the flag through the whole method and could instead return early, when needed.

willshowell · 2017-11-02T19:21:17Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+      let isEscapedHtmlTag = true;
+      const matches = new RegExp(/[<>]/);
+
+      const numberOfBackticks = fullText.split('`').length - 1;


backtickCount?

willshowell · 2017-11-02T19:21:59Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+           * backtick is found in the string, and it is set back to false when the next matching
+           * (closing) backtick is found. Every backtick must have a matching. < and >
+           * must always be between two matching backticks.
+           */


Use // single-line comments

Also I think only the last sentence is needed.

willshowell · 2017-11-02T19:22:50Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+           * must always be between two matching backticks.
+           */
+          const splitedFullText = fullText.split('');
+          let isBacktickWithoutMatch = false;


Maybe

// Whether an opening backtick has been found without a closing pair let openBacktick = false;

willshowell · 2017-11-02T19:23:03Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+           * (closing) backtick is found. Every backtick must have a matching. < and >
+           * must always be between two matching backticks.
+           */
+          const splitedFullText = fullText.split('');


fullTextArray?

willshowell · 2017-11-02T19:23:15Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+          const splitedFullText = fullText.split('');
+          let isBacktickWithoutMatch = false;
+
+          for (var i = 0; i < splitedFullText.length; i++) {


let i = 0

willshowell · 2017-11-02T19:25:06Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+      // if there are no backticks and [<>], there's no need for any more checks
+      if ((numberOfBackticks > 1) && matches.test(fullText)) {
+        // if there are backticks there should be an even number of them
+        if (!!(numberOfBackticks % 2)) {


Can just use numberOfBackticks % 2 since you only care whether its zero or non-zero.

willshowell · 2017-11-02T20:07:14Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+        return false;
+      } else {
+        // < and > must always be between two matching backticks.
+        const fullTextArray = fullText.split('');


I think the logic could be tightened up like this,

const matches = new RegExp(/[<>]/); const backtickCount = fullText.split('`').length - 1; // An odd number of backticks or html without backticks is invalid if (backtickCount % 2 || (!backtickCount && matches.test(fullText))) { return false; } // Text without html is valid if (!matches.test(fullText)) { return true; } // < and > must always be between two matching backticks const fullTextArray = fullText.split('');

rafaelss95 · 2017-11-03T02:17:08Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+
+  /** Gets whether the comment's HTML, if any, is properly escaped */
+  parseForHtml(fullText) {
+    const matches = new RegExp(/[<>]/);


Can be simplified to const matches = /[<>]/;

rafaelss95 · 2017-11-03T02:22:30Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+    const backtickCount = fullText.split('`').length - 1;
+
+    // An odd number of backticks or html without backticks is invalid
+    if ((backtickCount % 2) || ((backtickCount === 0) && matches.test(fullText))) {


I think there are too many parenthesis. It could be: if (backtickCount % 2 || (backtickCount === 0 && matches.test(fullText)) {

Also, you can just do !backtickCount as @willshowell mentioned in his comment.

I missed it.

rafaelss95 · 2017-11-03T02:40:25Z

tools/tslint-rules/noUnscapedHtmlTagRule.js

+    // Whether an opening backtick has been found without a closing pair
+    let openBacktick = false;
+
+    for (let i = 0; i < fullTextArray.length; i++) {


You can use for..of and loop over the string, so you don't need to use split('') above.

willshowell · 2017-11-03T12:47:06Z

@julianobrasil one last thing I noticed: since you opened the original PR, the other linters have been rewritten in typescript. Might want to convert it and make sure it matches the updated format of the other ones.

julianobrasil · 2017-11-03T15:18:07Z

Travis output bellow. Apparently this lint rule is doing what it is supposed to do (there was a little mistake of mine in the code in the part that capture the comment to be tested, but it's fixed now).

willshowell · 2017-11-03T15:22:03Z

Awesome! You can also run npm run tslint locally to lint. And I think those fixes should be included with this PR.

julianobrasil · 2017-11-03T15:47:52Z

@willshowell, locally I got a lot of lint error messages because windows line breaks are different from LF.

crisbeto

LGTM. @julianobrasil you should be able to switch the line endings in your IDE. If you're using vscode, there should be a "CRLF" button in the bottom right corner.

jelbourn · 2018-01-24T20:05:04Z

@julianobrasil could you rebase this PR?

@willshowell

- Address @willshowell's comments

julianobrasil · 2018-01-24T20:34:42Z

Done.

jelbourn · 2018-01-24T21:23:37Z

tslint.json

@@ -118,7 +118,8 @@
    "missing-rollup-globals": [
      true,
      "./tools/package-tools/rollup-globals.ts",
-      "src/+(lib|cdk|material-examples|material-experimental|cdk-experimental)/**/*.ts"
-    ]
+      "src/+(lib|cdk|material-examples)/**/*.ts"


Did you mean to remove material-experimental|cdk-experimental here?

Absolutely not. This was an oversight.

jelbourn

LGTM

jelbourn · 2018-01-24T22:02:16Z

@julianobrasil ah, this actually catches a pretty large number of instances already in the codebase. Did you want to send a separate PR that just removes those first so we can merge this and still have everything be green?

julianobrasil · 2018-01-24T22:21:17Z

I'll do it.

jelbourn · 2018-01-25T22:24:24Z

Looks like there are still two things being caught:

ERROR: /home/travis/build/angular/material2/src/lib/chips/chip-input.ts[24, 1]: An HTML tag delimiter (< or >) may only appear in a JSDoc comment if it is escaped. This prevents failures in document generation caused by a misinterpreted tag.
ERROR: /home/travis/build/angular/material2/src/lib/icon/icon.ts[33, 1]: An HTML tag delimiter (< or >) may only appear in a JSDoc comment if it is escaped. This prevents failures in document generation caused by a misinterpreted tag.

julianobrasil · 2018-01-26T00:42:37Z

This gave me more trouble than I expected. @jelbourn, sorry for all of this commiting ping-pong, but after updating my local copy of material repo on wednesday, I got a lot of errors with npm install. And this is preventing me from running anything locally (not sure yet what the problem is, but I'm trying to talk to the guys of grpc/grpc-node repo about it as it seems to be the culprit)

jelbourn · 2018-01-26T00:48:21Z

This one passes now, so I'm merging it

angular-automatic-lock-bot · 2019-09-08T05:02:45Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

julianobrasil requested review from devversion and jelbourn as code owners November 2, 2017 18:37

googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Nov 2, 2017

willshowell reviewed Nov 2, 2017

View reviewed changes

jelbourn requested a review from crisbeto November 2, 2017 22:23

rafaelss95 reviewed Nov 3, 2017

View reviewed changes

crisbeto approved these changes Nov 3, 2017

View reviewed changes

julianobrasil added 5 commits January 24, 2018 18:32

build: tslint rule to enforce html tags escaping in comments

9aeff89

- Address @willshowell's comments

address comments

09688ba

tightening the logic

02dda8d

address @rafaelss95's comments

7cd8cff

switch to typescript

1b03d22

jelbourn reviewed Jan 24, 2018

View reviewed changes

bring back material-experimental|cdk-experimental

03b541f

jelbourn approved these changes Jan 24, 2018

View reviewed changes

julianobrasil mentioned this pull request Jan 24, 2018

build: escape html tags and close unclosed backticks #9583

Merged

jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker labels Jan 25, 2018

julianobrasil mentioned this pull request Jan 26, 2018

build: fix remaining unescaped html tags #9611

Merged

jelbourn merged commit c306297 into angular:master Jan 26, 2018

julianobrasil deleted the tslint-html-rule branch January 26, 2018 00:50

jelbourn added the target: patch This PR is targeted for the next patch release label Jan 29, 2018

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 8, 2019

build: tslint rule to enforce html tags escaping in comments #8200

build: tslint rule to enforce html tags escaping in comments #8200

Uh oh!

Conversation

julianobrasil commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willshowell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rafaelss95 Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willshowell commented Nov 3, 2017

Uh oh!

julianobrasil commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willshowell commented Nov 3, 2017

Uh oh!

julianobrasil commented Nov 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

crisbeto left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jan 24, 2018

Uh oh!

julianobrasil commented Jan 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jelbourn left a comment

Choose a reason for hiding this comment

Uh oh!

jelbourn commented Jan 24, 2018

Uh oh!

julianobrasil commented Jan 24, 2018

Uh oh!

jelbourn commented Jan 25, 2018

Uh oh!

julianobrasil commented Jan 26, 2018

Uh oh!

jelbourn commented Jan 26, 2018

Uh oh!

angular-automatic-lock-bot bot commented Sep 8, 2019

Uh oh!

julianobrasil commented Nov 2, 2017 •

edited

Loading

rafaelss95 Nov 3, 2017 •

edited

Loading

julianobrasil commented Nov 3, 2017 •

edited

Loading

julianobrasil commented Nov 3, 2017 •

edited

Loading