-
Notifications
You must be signed in to change notification settings - Fork 138
Fix multiple <svg>
and missing content issue
#2722
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2722 +/- ##
========================================
Coverage 52.84% 52.84%
========================================
Files 130 130
Lines 7162 7162
Branches 1503 1572 +69
========================================
Hits 3785 3785
- Misses 3072 3222 +150
+ Partials 305 155 -150 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
<svg>
and missing content issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue with multiple elements triggering redundant code injection, resulting in an incorrect Content-Length header that causes missing content in HTML responses.
- Adjusts the regex for matching tags to count all occurrences
- Corrects the Content-Length calculation by multiplying the injection code length by the number of matches
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR addresses the cause of HTML content being cut-off when being served by live-server
, specifically in the case where there are more than 1 <svg>
files.
Some things to perhaps consider beyond the scope of this PR or project:
-
If my understanding is correct, the original package implementation only intended to inject the svg script once, but inadvertently injected multiple duplicate scripts. This fix accounts for this side effect of duplicate injection, but does not inherently resolve the duplicate injection.
- (Which could have performance impacts in some small edge cases, e.g. 999x
<svg>
served in a html file)
- (Which could have performance impacts in some small edge cases, e.g. 999x
-
In the event that the original package is updated to resolve this issue, it will have to manually updated through MarkBind's patch in this file, which should be made known to future maintainers that may read this PR.
Overall, this PR successfully resolves the bug through the existing patch of the live-server
package. LGTM. Thank you for your help! @AgentHagu
@AgentHagu @gerteck Thanks for this fix. Shall we do release a minor version including this fix (and any other recent/ready fixes)? I'm still waiting to upgrade to V6 due to this issue. |
Will follow up on this! I think I will need to get the appropriate permissions on NPM to make a new release |
Tried v6.0.1. #2715 seems to be fixed now. Thanks everyone! |
What is the purpose of this pull request?
Overview of changes:
Fixes #2715
The code updates MarkBind's existing patch of
live-server
to account for the number of<svg>
s in the HTML file and adjusts the Content-Length header accordingly.Anything you'd like to highlight/discuss:
Thanks to @gerteck for finding the root cause and a fix for it.
The root cause of the issue was the code injection within the
live-server
package. It would normally detect<body>
,<svg>
or<head>
and inject the<script>
for live-server there. It would also increase the Content-Length by the length of the injected code.The issue came about because of the use of
es.replace()
. According to theevent-stream
npm page, rather than replacing just the first occurrence, it replaces all matching occurrences. This meant that when a file had multiple<svg>
s, it would inject code into all of them but only increase the Content-Length once. This lead to the resulting HTML response being cutoff and causing missing content.Testing instructions:
npm run build:backend
test.md
, include a codeblock, such as:codeBlockCopyButtons
andcodeBlockWrapButtons
plugins (or replace the codeblock with >2<svg>
s)<svg>
s)Proposed commit message: (wrap lines at 72 characters)
Fix multiple
<svg>
and missing content issuelive-server
injects code into every<svg>
occurrence but onlyincreases the Content-Length value once. This commit fixes this
issue by counting the number of
<svg>
and increasing theContent-Length accordingly, ensuring that all content is properly
sent.
Checklist: ☑️
Reviewer checklist:
Indicate the SEMVER impact of the PR:
At the end of the review, please label the PR with the appropriate label:
r.Major
,r.Minor
,r.Patch
.Breaking change release note preparation (if applicable):