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

rschristian · 2023-09-04T04:48:22Z

Description

Additional context

Let me know if a test case is desired, and how I'm to add one. I did take a look around but found no obvious location; while playground/html exists, it appears to be entirely for browser/integration tests.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the PR Title Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

stackblitz · 2023-09-04T04:48:24Z

Run & review this pull request in StackBlitz Codeflow.

sapphi-red · 2023-09-04T06:55:59Z

Won't this break HTMLs like this?

<div style="white-space: pre">
  foo
  bar
</div>

I guess changing s.remove part could work.

rschristian · 2023-09-04T07:05:53Z

Ah, shoot, yeah.

I didn't love the idea of changing s.remove as you'd have to extrapolate new start positions for each node to include the preceding whitespace, but maybe that's the correct solution. Will look into it, thanks.

…k lines

rschristian · 2023-09-04T08:58:02Z

packages/vite/src/node/plugins/html.ts

+        const nodeStartWithLeadingWhitespace = (
+          node: DefaultTreeAdapterMap['node'],
+        ) => {
+          const lineStartOffset =
+            node.sourceCodeLocation!.startOffset -
+            node.sourceCodeLocation!.startCol
+          const line = s.slice(
+            lineStartOffset,
+            node.sourceCodeLocation!.startOffset,
+          )
+          return /\S/.test(line)
+            ? node.sourceCodeLocation!.startOffset
+            : lineStartOffset
+        }


Name could probably be better, open to suggestions! My brain is failing me at the moment though.

Simply checks if the content preceding the startOffset on the line is white space only, and if so, moves the startOffset to the start of the line so we remove it too.

packages/vite/src/node/plugins/html.ts

Co-authored-by: 翠 / green <green@sapphi.red>

bluwy · 2023-10-05T09:04:03Z

packages/vite/src/node/plugins/html.ts

+            lineStartOffset,
+            node.sourceCodeLocation!.startOffset,
+          )
+          return /\S/.test(line)


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

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.

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.

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.

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.

BenceSzalai · 2023-10-12T00:42:10Z

My project that works fine with beta.4 is broken with beta.5 (and beta.6). I'm not sure, but I am suspicious it is related to this PR.

[vite:build-html] end must be greater than start
file: /path/to/project/apps/client/index.html
file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:16180
                if (start > end) throw new Error('end must be greater than start');
                                       ^

Error: end must be greater than start
    at MagicString.remove (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:16180:26)
    at file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:38222:27
    at traverseNodes (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37931:5)
    at file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37935:48
    at Array.forEach (<anonymous>)
    at traverseNodes (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37935:25)
    at file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37935:48
    at Array.forEach (<anonymous>)
    at traverseNodes (file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37935:25)
    at file:///path/to/project/node_modules/vite/dist/node/chunks/dep-db07a1ea.js:37935:48 {
  code: 'PLUGIN_ERROR',
  plugin: 'vite:build-html',
  hook: 'transform',
  id: '/path/to/project/apps/client/index.html',
  watchFiles: [
    '/path/to/project/apps/client/index.html'
  ]
}

Node.js v20.5.1
error Command failed with exit code 1.

What do you think?

rschristian · 2023-10-12T00:43:43Z

@BenceSzalai Can you provide a reproduction? That seems like a reasonable suspicion, but I can't think of any failing situations.

BenceSzalai · 2023-10-12T01:10:23Z

@rschristian: It is not trivial to make a reproduction because it is a Quasar project, so the Vite configuration is behind layers of abstraction.

But what I can see looking at the exact lines in the stack trace is that this change is the cause:
https://github.com/vitejs/vite/pull/14274/files#diff-89bae1df62862bb7f4a03d82a1e9cbf4ac6d0c042f21fbbacb0a2238bd050042R552

Namely nodeStartWithLeadingWhitespace(node) gives -1 in my case.

Probably it is because my input HTML has no linebreaks at all. So that is an edge case not handled properly I guess.

If I replace this line with Math.max(0,nodeStartWithLeadingWhitespace(node)) the build succeeds, albeit produces broken HTML with large portion missing in the beginning. For a proper fix it needs to be looked into why it ends up calculating -1, but this should be enough I think to reproduce the issue.

The main thing is try to begin with a HTML where all line-breaks are removed before given to Vite.

rschristian · 2023-10-12T01:14:26Z

It is not trivial to make a reproduction because it is a Quasar project, so the Vite configuration is behind layers of abstraction.

That shouldn't matter, we're just looking for the HTML that actually gets passed to this plugin. Inserting a log statement for the html here would probably be enough to spot the issue, if there is one.

BenceSzalai · 2023-10-12T01:15:44Z

I did that exactly, that's how I've found out that Quasar removes all linebreaks before the HTML is given to Vite:

<!DOCTYPE html><html><head><title>Web Client</title><meta charset=utf-8><meta name=description content="A new application"><meta name=format-detection content="telephone=no"><meta name=msapplication-tap-highlight content=no><meta name=viewport content="user-scalable=no,initial-scale=1,maximum-scale=1,minimum-scale=1,width=device-width"><link rel=icon type=image/png sizes=128x128 href=/icons/favicon-128x128.png><link rel=icon type=image/png sizes=96x96 href=/icons/favicon-96x96.png><link rel=icon type=image/png sizes=32x32 href=/icons/favicon-32x32.png><link rel=icon type=image/png sizes=16x16 href=/icons/favicon-16x16.png><link rel=icon type=image/ico href=/favicon.ico></head><body><div id=q-app></div><script type=module src=/.quasar/client-entry.js></script></body></html>

And apparently the changed code cannot any more handle this.

rschristian · 2023-10-12T01:18:34Z

Perfect, thank you. Will look into it a bit later.

BenceSzalai · 2023-10-12T01:19:28Z

Great, thanks for the quick response!

rschristian · 2023-10-12T02:08:43Z

Got it, need to Math.max the lineStartOffset as the column isn't 0-indexed. Will push a fix in a minute.

Thanks for the HTML, helped track this down much quicker!

refactor: Ensure HTML is stripped of generated blank lines

9deeec9

style: Fix formatting

929074f

rschristian marked this pull request as draft September 4, 2023 07:05

rschristian added 3 commits September 4, 2023 03:33

revert: Undo 9deeec9

c1fad94

refactor: Better method of ensuring generated HTML doesn't leave blan…

3099574

…k lines

fix: Correct type signature?

1404bd2

rschristian force-pushed the chore/html-whitespace-removal branch from ca17f56 to 1404bd2 Compare September 4, 2023 08:51

rschristian commented Sep 4, 2023

View reviewed changes

fix: Account for HTML that starts with to be removed node

e756d57

rschristian commented Sep 4, 2023

View reviewed changes

packages/vite/src/node/plugins/html.ts Outdated Show resolved Hide resolved

rschristian marked this pull request as ready for review September 4, 2023 10:44

rschristian force-pushed the chore/html-whitespace-removal branch from 97c335b to e756d57 Compare September 5, 2023 02:33

sapphi-red added p2-nice-to-have Not breaking anything but nice to have (priority) feat: html labels Sep 6, 2023

sapphi-red reviewed Sep 6, 2023

View reviewed changes

packages/vite/src/node/plugins/html.ts Outdated Show resolved Hide resolved

packages/vite/src/node/plugins/html.ts Show resolved Hide resolved

rschristian and others added 2 commits September 6, 2023 17:33

chore: Correct equality check & add comment

645fbbf

Co-authored-by: 翠 / green <green@sapphi.red>

chore: Update comment

9754e25

rschristian force-pushed the chore/html-whitespace-removal branch from b1be990 to 9754e25 Compare September 10, 2023 08:45

sapphi-red previously approved these changes Sep 10, 2023

View reviewed changes

bluwy reviewed Oct 5, 2023

View reviewed changes

refactor: Update content detection, add clarifying comments

76327fb

rschristian dismissed sapphi-red’s stale review via 76327fb October 9, 2023 05:33

bluwy approved these changes Oct 9, 2023

View reviewed changes

sapphi-red approved these changes Oct 9, 2023

View reviewed changes

sapphi-red enabled auto-merge (squash) October 9, 2023 08:52

sapphi-red merged commit bc97091 into vitejs:main Oct 9, 2023
9 checks passed

rschristian deleted the chore/html-whitespace-removal branch October 12, 2023 00:43

rschristian mentioned this pull request Oct 12, 2023

fix: off-by-one bug in HTML whitespace removal #14589

Merged

9 tasks

BenceSzalai mentioned this pull request Oct 12, 2023

Vite 3/4/5 support for Quasar CLI with Vite quasarframework/quasar#14077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rschristian commented Sep 4, 2023

stackblitz bot commented Sep 4, 2023

sapphi-red commented Sep 4, 2023

rschristian commented Sep 4, 2023

rschristian Sep 4, 2023

bluwy Oct 5, 2023

rschristian Oct 5, 2023

bluwy Oct 5, 2023

rschristian Oct 5, 2023

rschristian Oct 9, 2023

BenceSzalai commented Oct 12, 2023

rschristian commented Oct 12, 2023 •

edited

Loading

BenceSzalai commented Oct 12, 2023 •

edited

Loading

rschristian commented Oct 12, 2023

BenceSzalai commented Oct 12, 2023 •

edited

Loading

rschristian commented Oct 12, 2023

BenceSzalai commented Oct 12, 2023

rschristian commented Oct 12, 2023

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

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

Conversation

rschristian commented Sep 4, 2023

Description

Additional context

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

stackblitz bot commented Sep 4, 2023

sapphi-red commented Sep 4, 2023

rschristian commented Sep 4, 2023

rschristian Sep 4, 2023

Choose a reason for hiding this comment

bluwy Oct 5, 2023

Choose a reason for hiding this comment

rschristian Oct 5, 2023

Choose a reason for hiding this comment

bluwy Oct 5, 2023

Choose a reason for hiding this comment

rschristian Oct 5, 2023

Choose a reason for hiding this comment

rschristian Oct 9, 2023

Choose a reason for hiding this comment

BenceSzalai commented Oct 12, 2023

rschristian commented Oct 12, 2023 • edited Loading

BenceSzalai commented Oct 12, 2023 • edited Loading

rschristian commented Oct 12, 2023

BenceSzalai commented Oct 12, 2023 • edited Loading

rschristian commented Oct 12, 2023

BenceSzalai commented Oct 12, 2023

rschristian commented Oct 12, 2023

rschristian commented Oct 12, 2023 •

edited

Loading

BenceSzalai commented Oct 12, 2023 •

edited

Loading

BenceSzalai commented Oct 12, 2023 •

edited

Loading