Skip to content

warn if comments are removed from HTML #8423

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

Merged
Rich-Harris merged 7 commits into from
Jan 10, 2023
Merged

warn if comments are removed from HTML #8423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bcad8de
warn if comments are removed from HTML - closes #7493
Rich-Harris Jan 9, 2023
3de6e3a
use esm-env
Rich-Harris Jan 9, 2023
d5659b9
Update packages/kit/src/runtime/server/page/render.js
Rich-Harris Jan 9, 2023
a4d2439
add explanatory comment
Rich-Harris Jan 9, 2023
73ce487
Merge branch 'gh-7493' of github.com:sveltejs/kit into gh-7493
Rich-Harris Jan 9, 2023
90dc980
prettier
Rich-Harris Jan 10, 2023
cc4644a
only warn if csr is true
dummdidumm Jan 10, 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
5 changes: 5 additions & 0 deletions .changeset/cuddly-camels-hang.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Warn if comments are removed from HTML
26 changes: 22 additions & 4 deletions packages/kit/src/runtime/server/page/render.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { s } from '../../../utils/misc.js';
import { Csp } from './csp.js';
import { uneval_action_response } from './actions.js';
import { clarify_devalue_error } from '../utils.js';
import { DEV } from 'esm-env';

// TODO rename this function/module

Expand Down Expand Up @@ -353,17 +354,34 @@ export async function render_response({
// add the content after the script/css links so the link elements are parsed first
head += rendered.head;

const html = options.app_template({
head,
body,
assets,
nonce: /** @type {string} */ (csp.nonce)
});

// TODO flush chunks as early as we can
const html =
const transformed =
(await resolve_opts.transformPageChunk({
html: options.app_template({ head, body, assets, nonce: /** @type {string} */ (csp.nonce) }),
html,
done: true
})) || '';

if (DEV && page_config.csr) {
if (transformed.split('<!--').length < html.split('<!--').length) {
Copy link
Member

@benmccann benmccann Jan 9, 2023

Choose a reason for hiding this comment

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

something like transformed.match(/<!--/g).length might be less expensive as i think it wouldn't create large arrays (i'm not a regex expert - that might need some escaping or something)

Copy link
Member

@Conduitry Conduitry Jan 9, 2023

Choose a reason for hiding this comment

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

String.prototype.match called with a global regex does return a regular array anyway (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/match), so unless there's some clever optimization that works in one case but not the other, we're not really avoiding anything by doing that.

I don't think we want to worry overly much about performance here anyway, because it is a dev-only check,

Copy link
Member Author

@Rich-Harris Rich-Harris Jan 9, 2023

Choose a reason for hiding this comment

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

huh TIL about match. What a fantastically useless API! Struggling to imagine a single use case for it other than ours — counting — for which allocating an array is incredibly wasteful. What the hell am I supposed to do with the returned strings, without indexes etc?

It's .match(string) that really kills me though — it will return an array but it will only ever have a length of 1 and its value will only ever be [input]:

// WHAT THE FUCK AM I SUPPOSED TO DO WITH THIS?!
'wtf'.match('wtf'); // ['wtf']

Anyway, this was a good hypothesis but I just ran some microbenchmarks...

const html = `<!-- contents of view-source:https://kit.svelte.dev/ -->`

let iterations = 1e6;

function with_split() {
  let i = iterations;
  let count;
  console.time('with split');
  while (i--) {
    count = html.split('<!--').length;
  }
  console.timeEnd('with split');
}

function with_match() {
  let i = iterations;
  let count;
  console.time('with match');
  while (i--) {
    count = html.match(/<!--/g).length;
  }
  console.timeEnd('with match');
}

with_split();
with_match();

...and the match version is a full two orders of magnitude slower (7.4s vs 69ms — nice — on my machine).

// the \u001B stuff is ANSI codes, so that we don't need to add a library to the runtime
// https://svelte.dev/repl/1b3f49696f0c44c881c34587f2537aa2
console.warn(
Copy link
Member

@dummdidumm dummdidumm Jan 10, 2023

Choose a reason for hiding this comment

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

I think we should emit this warning only if csr is true (as mentioned in the linked issue), since if we don't need hydration if it's turned off (there's no JS delivered to the page)

"\u001B[1m\u001B[31mRemoving comments in transformPageChunk can break Svelte's hydration\u001B[39m\u001B[22m"
);
}
}

const headers = new Headers({
'x-sveltekit-page': 'true',
'content-type': 'text/html',
etag: `"${hash(html)}"`
etag: `"${hash(transformed)}"`
});

if (!state.prerendering) {
Expand All @@ -381,7 +399,7 @@ export async function render_response({
}
}

return new Response(html, {
return new Response(transformed, {
status,
headers
});
Expand Down