warn if comments are removed from HTML #8423
Conversation
🦋 Changeset detectedLatest commit: cc4644a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| })) || ''; | ||
| | ||
| if (DEV) { | ||
| if (transformed.split('<!--').length < html.split('<!--').length) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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).
| if (transformed.split('<!--').length < html.split('<!--').length) { | ||
| // 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( |
There was a problem hiding this comment.
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)
closes #7493. I don't 100% know if this is the right approach — it is fairly crude, and could yield false positives and false negatives — or if the message here is sufficiently self-explanatory. But it's probably better than nothing.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0