feat(legacy-html): don't minify assets, add assets to c8 exclude #323
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the legacy HTML generator to improve performance by moving inline CSS/JS into external files and by simplifying the HTML minification process.
- Removed inline dark-mode CSS from the HTML template and moved it to an external stylesheet.
- Removed JS/CSS minification options in favor of simpler HTML minification in both legacy-html generators.
- Refactored assets/api.js to modernize functions with arrow syntax and improve event handling, and updated ESLint config for newer ECMAScript features.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/generators/legacy-html/template.html | Removed inline style block for dark mode styling |
| src/generators/legacy-html/index.mjs | Removed JS/CSS minification options to speed up HTML generation |
| src/generators/legacy-html/assets/style.css | Added dark-mode CSS styling rules to external stylesheet |
| src/generators/legacy-html/assets/api.js | Refactored functions with modern syntax and improved event handling |
| src/generators/legacy-html-all/index.mjs | Removed JS/CSS minification options for the combined legacy HTML generation |
| eslint.config.mjs | Updated ESLint configuration to use the latest ECMAScript version |
Comments suppressed due to low confidence (1)
eslint.config.mjs:79
- [nitpick] Consider adding a note in the project documentation about the required ECMAScript/Node version to support 'latest' language features configured here.
languageOptions: { globals: { ...globals.browser }, ecmaVersion: 'latest' },
avivkeller
commented
Jun 26, 2025
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #323 +/- ##
==========================================
+ Coverage 66.57% 71.25% +4.67%
==========================================
Files 81 117 +36
Lines 6956 9650 +2694
Branches 339 585 +246
==========================================
+ Hits 4631 6876 +2245
- Misses 2322 2771 +449
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I don't think ESBuild minifies HtML |
|
Hum sad 😔 |
avivkeller
changed the title
avivkeller
force-pushed
the
perf/minify-speed
branch
from
9886eef to
78214e3
Compare
AugustinMauroy
approved these changes
Jun 26, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
By not having terser
minifyCSSorminifyJS, we save ~7 seconds on generation. See also #320. After this, Shiki is the hottest code path, followed byparseApiDoc.Also, while I'm changing the assets, I've added them to the coverage exclude, since we aren't planning to test them, they are browser-based. This PR crosses the coverage threshold of 70%, so that was also changed.