refactor: add cache for template literal parsing #390
Conversation
| @@ -20,7 +20,8 @@ | |||
| * @property {Record<string, number>} [Option2.tagChildrenIndent] | |||
| */ | |||
|
|
|||
| const { parse } = require("@html-eslint/template-parser"); | |||
| const { getCachedParseResult } = require("../utils/template-cache"); | |||
There was a problem hiding this comment.
An alternative implementation would be to make caching a part of the parse function itself so that this is completely transparent to rules. I opted to go this route to avoid making changes that might be undesirable to anyone who is using the @html-eslint/template-parser package in isolation.
nwalters512
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR improves performance by introducing a per-node cache for parsed template literals and switching rule implementations to use the cached AST plus a separate traversal step rather than reparsing on each rule invocation.
- Added
template-cache.jsto store parse results in a WeakMap keyed by AST nodes. - Updated core visitor utilities (
visitors.js) and all affected rules to replace directparsecalls withgetCachedParseResult+traverse. - Removed direct imports of
parsefrom@html-eslint/template-parserin favor of the new caching layer.
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/eslint-plugin/lib/rules/utils/template-cache.js | New module providing WeakMap-based caching of parse results |
| packages/eslint-plugin/lib/rules/utils/visitors.js | Switched from parse to getCachedParseResult + traverse |
| packages/eslint-plugin/lib/rules/no-trailing-spaces.js | Updated to use cached parse results |
| packages/eslint-plugin/lib/rules/no-multiple-empty-lines.js | Updated to use cached parse results |
| packages/eslint-plugin/lib/rules/no-duplicate-in-head.js | Updated to use cached parse results and traverse |
| packages/eslint-plugin/lib/rules/no-duplicate-id.js | Updated to use cached parse results and traverse |
| packages/eslint-plugin/lib/rules/indent/indent.js | Updated to use cached parse results and traverse |
Comments suppressed due to low confidence (2)
packages/eslint-plugin/lib/rules/utils/template-cache.js:2
- [nitpick] JSDoc tag
@importis not a standard annotation. Consider using@typedefor@templateto reference these types so editors and type-checkers recognize them.
* @import {TemplateLiteral} from "@html-eslint/types";
packages/eslint-plugin/lib/rules/utils/template-cache.js:30
- There are no unit tests verifying that
getCachedParseResultactually caches parse results. Consider adding tests to ensure the cache is used andparseis invoked only once per node.
const result = parse(node, sourceCode, {});
| @@ -6,7 +6,8 @@ const { | |||
| shouldCheckTaggedTemplateExpression, | |||
| shouldCheckTemplateLiteral, | |||
| } = require("./settings"); | |||
| const { parse } = require("@html-eslint/template-parser"); | |||
| const { getCachedParseResult } = require("./template-cache"); | |||
| const { traverse } = require("@html-eslint/template-parser/lib/traverser"); | |||
There was a problem hiding this comment.
[nitpick] The traverse import and usage pattern is duplicated across multiple rule files. Consider centralizing a helper that wraps parse + traverse to reduce boilerplate and ease future updates.
Copilot uses AI. Check for mistakes.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #390 +/- ##
==========================================
- Coverage 98.36% 98.30% -0.07%
==========================================
Files 81 82 +1
Lines 2635 2657 +22
Branches 725 726 +1
==========================================
+ Hits 2592 2612 +20
- Misses 43 45 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Hi @nwalters512 Thank you for your contribution — great work! I think there's some room for a bit of refactoring. Would it be alright if I continue with that after merging this PR? |
|
Absolutely! Feel free to run with this as a starting point. And let me know if there's anything I can do to help. My team would love to use this plugin so the sooner we can get a release with these improvements the better 🙂 |
|
Thanks! I'm planning to release a version including this PR this weekend. |
Adding per-node caching of parse results gives a 94% performance improvement in my project. Before, the rules collectively took ~21s. Now, they take just ~1.2s.
Before:
After:
This code was developed with the help of Claude Sonnet 4.