Skip to content
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.

Sign up for GitHub

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

Jump to bottom

refactor: ensure HTML is stripped of generated blank lines #14274

Merged
merged 9 commits into from
Oct 9, 2023
Merged

refactor: ensure HTML is stripped of generated blank lines #14274

Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
9deeec9
refactor: Ensure HTML is stripped of generated blank lines
rschristian Sep 4, 2023
929074f
style: Fix formatting
rschristian Sep 4, 2023
c1fad94
revert: Undo 9deeec9
rschristian Sep 4, 2023
3099574
refactor: Better method of ensuring generated HTML doesn't leave blan…
rschristian Sep 4, 2023
1404bd2
fix: Correct type signature?
rschristian Sep 4, 2023
e756d57
fix: Account for HTML that starts with to be removed node
rschristian Sep 4, 2023
645fbbf
chore: Correct equality check & add comment
rschristian Sep 6, 2023
9754e25
chore: Update comment
rschristian Sep 10, 2023
76327fb
refactor: Update content detection, add clarifying comments
rschristian Oct 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/vite/src/node/plugins/html.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,6 +323,22 @@
config,
publicToRelative,
)
const nodeStartWithLeadingWhitespace = (
node: DefaultTreeAdapterMap['node'],
) => {
if (node.sourceCodeLocation!.startOffset == 0)

Check warning on line 329 in packages/vite/src/node/plugins/html.ts

View workflow job for this annotation

GitHub Actions / Lint: node-18, ubuntu-latest 

Expected '===' and instead saw '=='
rschristian marked this conversation as resolved.
Show resolved Hide resolved
return node.sourceCodeLocation!.startOffset
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
const lineStartOffset =
node.sourceCodeLocation!.startOffset -
node.sourceCodeLocation!.startCol
sapphi-red marked this conversation as resolved.
Show resolved Hide resolved
const line = s.slice(
lineStartOffset,
node.sourceCodeLocation!.startOffset,
)
return /\S/.test(line)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can use line.trim() instead as it's more performant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose? Personally, I'd find that to be quite unclear and the performance gain is non-existent for this specific use case. You'd need thousands of tags to make a measurable difference.

If that's what you want though I'll switch it over.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It also took me a while to understand what /\S/ does 😅 I'd prefer .trim() and also put a comment above to explain what we're doing. Every little performance counts and they stack over time if we're not careful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't love \S either tbh, but that's on your linter! 😅 I believe it forced [^\s] to that, which, fair enough, though I wasn't familiar with \S myself.

Will update sometime tomorrow, pretty late here for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, took a bit longer to get to this. Switched over the detection and added comments hopefully clarifying which situations are being covered, lmk if you'd like to see any further revisions.

? node.sourceCodeLocation!.startOffset
: lineStartOffset
}

// pre-transform
html = await applyHtmlTransforms(html, preHooks, {
Expand Down Expand Up @@ -444,7 +460,7 @@
const importExpression = `\nimport ${JSON.stringify(url)}`
styleUrls.push({
url,
start: node.sourceCodeLocation!.startOffset,
start: nodeStartWithLeadingWhitespace(node),
end: node.sourceCodeLocation!.endOffset,
})
js += importExpression
Expand Down Expand Up @@ -516,7 +532,7 @@
// remove the script tag from the html. we are going to inject new
// ones in the end.
s.remove(
node.sourceCodeLocation!.startOffset,
nodeStartWithLeadingWhitespace(node),
node.sourceCodeLocation!.endOffset,
)
}
Expand Down