fix(eslint-plugin): support template literals in no-html-links rule#11907
fix(eslint-plugin): support template literals in no-html-links rule#11907Injora wants to merge 2 commits intofacebook:mainfrom
Conversation
✅ [V2]Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
slorber
left a comment
slorber
left a comment
There was a problem hiding this comment.
I don't think this is what I had in mind when adding this todo comment.
isFullyResolvedUrl("${prefix}dummy.com") is unlikely to be a good solution unless you can explain to me why. I'm pretty sure this creates false positives that are not currently covered by tests. More tests should be added to cover more complex template literal cases
| const prefix = String(firstQuasi.value.raw); | ||
| if ( | ||
| isFullyResolvedUrl(prefix) || | ||
| isFullyResolvedUrl(`${prefix}dummy.com`) |
There was a problem hiding this comment.
I don't understand, what is dummy.com?
There was a problem hiding this comment.
Thanks for the feedback! You're right, the dummy.com hack was fragile.
I've refactored the rule to use a more robust staticPreEvaluate utility function that recursively resolves TemplateLiteral, Literal, and BinaryExpression (string concatenation) nodes at static analysis time. This allows us to correctly verify if a generated string is a fully-resolved external URL without relying on string prefixes.
I've also added several new test cases:
Statically evaluatable template literals (e.g., https://x.com/${"docu" + "saurus"}) are now correctly allowed.
String concatenation (e.g., "https://x.com/" + "docusaurus") is also supported.
Dynamic template literals (e.g., https://github.com/${repo}) are now explicitly tested to ensure they are still reported as invalid, since their final URL cannot be statically verified.
Let me know if this approach aligns better with what you had in mind :)
| { | ||
| // 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://x.com/${"docu" + "saurus"} ${"rex"}`}>Twitter</a>', | ||
| options: [{ignoreFullyResolved: true}], | ||
| errors: errorsJSX, |
There was a problem hiding this comment.
We don't have a unit test for when the template literals are dynamic and can't be resolved at static analysis time: we should add one to ensure it keeps failing
2 participants
Pre-flight checklist
Motivation
This pull request resolves a
// TODOin theno-html-linksESLint rule regardingTemplateLiteralparsing. Previously, the rule could not statically determine the validity of a URL if a template literal contained interpolated JSX expressions (e.g.,<a href={\https://example.com/docs/\${id}\`} />`), causing it to fail.This change introduces a base URL prefix check. By evaluating the string segment before the first variable to ensure it contains a valid protocol (
https://,mailto:, etc.), the parser can now correctly allow fully resolved external links written as template literals.Test Plan
packages/eslint-plugin/src/rules/no-html-links.tsto implement the prefix checking logic.validassertions array inpackages/eslint-plugin/src/rules/__tests__/no-html-links.test.ts.yarn test packages/eslint-plugin/src/rules/__tests__/no-html-links.test.ts.Test links
Deploy preview: https://deploy-preview-_____--docusaurus-2.netlify.app/
Related issues/PRs
N/A