fix: correctly escape backticks when precomputing CSS to inline#15593
Conversation
🦋 Changeset detectedLatest commit: 2d61d66 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 |
| assert.equal(escape_html('\ud800\ud800\udc00\udc00', true), '�\ud800\udc00�'); | ||
| }); | ||
|
|
||
| test('escape_for_interpolation escapes both backticks and dollar signs', () => { |
There was a problem hiding this comment.
We don't need this. The tests in utils.spec.js are more useful since they test that it works when the generated module is evaluated
| const assets_placeholder = '__SVELTEKIT_ASSETS__'; | ||
| const base_placeholder = '__SVELTEKIT_BASE__'; |
There was a problem hiding this comment.
Would there be any value in creating a quick util function like generate_placeholder(str, base) that you could call like generate_placeholder(css, '__SVELTEKIT_ASSETS__ ') that checks to see if the placeholder is in the string already, then adds some random stuff to the end of it in a loop if it is?
Super edge case but it's not impossible a CSS file includes these strings 😆 Might be more trouble than it's worth, though
There was a problem hiding this comment.
Probably! When I was chatting with Sonnet, it suggested using things like null-byte strings to make it more unique but that causes issues with the Svelte CSS parser. Let me see try one where we just suffix an incremental number or something
There was a problem hiding this comment.
Something that https://github.com/Rich-Harris/code-red did for Svelte 3/4-era compiler output was to just randomly choose at runtime a special placeholder string. That always felt safe enough to me. It prevented people from trying to screw up the compiler because there would be no way for them to guess or access what the special placeholder was going to be.
There was a problem hiding this comment.
Yeah this ^ was what I originally started at. But if you check the string for the placeholder ahead of time you also end up with the same result
There was a problem hiding this comment.
Okay one thing that just hit me -- we might want to do this in such a way that the result is stable, i.e. not random -- like starting from 1, then incrementing upwards. Mainly because if it's random, we can't cache an otherwise-identical asset between builds.
There was a problem hiding this comment.
There was a problem hiding this comment.
better, but maybe we should move the key initialization into the function itself? Otherwise if the function just so happens to be called in a different order it'll produce different results
There was a problem hiding this comment.
Mm makes sense. I've moved it in here 2d61d66 (this PR)
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
Looks good, just one thought
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated. # Releases ## @sveltejs/kit@2.56.0 ### Minor Changes - breaking: rework client-driven refreshes ([#15562](#15562)) - breaking: stabilize remote function caching by sorting object keys ([#15570](#15570)) - breaking: add `run()` method to queries, disallow awaiting queries outside render ([#15533](#15533)) - feat: support TypeScript 6.0 ([#15595](#15595)) - breaking: isolate command-triggered query refresh failures per-query ([#15562](#15562)) - feat: use `hydratable` for remote function transport ([#15533](#15533)) - feat: allow `form` fields to specify a default value (`field.as(type, value)`) ([#15577](#15577)) ### Patch Changes - fix: don't request new data when `.refresh` is called on a query with no cache entry ([#15533](#15533)) - fix: allow using multiple remote functions within one async derived ([#15561](#15561)) - fix: avoid false-positive overridden Vite `base` setting warning when setting a `paths.base` in `svelte.config.js` ([#15623](#15623)) - fix: manage queries in their own `$effect.root` ([#15533](#15533)) - fix: avoid `inlineDynamicImports` deprecation warning when building the service worker with Vite 8 ([#15550](#15550)) - fix: correctly escape backticks when precomputing CSS ([#15593](#15593)) - fix: discard obsolete forks before finishing navigation ([#15634](#15634)) - chore: tighten up override implementation ([#15562](#15562)) - fix: ensure the default Svelte 5 `error.svelte` file uses runes mode ([#15609](#15609)) - fix: deduplicate same-cache-key `batch` calls during SSR ([#15533](#15533)) - fix: decrement pending_count when form callback doesn't call submit() ([#15520](#15520)) Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
This PR contains the following updates: | Package | Change | [Age](https://docs.renovatebot.com/merge-confidence/) | [Confidence](https://docs.renovatebot.com/merge-confidence/) | |---|---|---|---| | [@sveltejs/kit](https://svelte.dev) ([source](https://github.com/sveltejs/kit/tree/HEAD/packages/kit)) | [`2.55.0` → `2.56.1`](https://renovatebot.com/diffs/npm/@sveltejs%2fkit/2.55.0/2.56.1) |  |  | --- ### Release Notes <details> <summary>sveltejs/kit (@​sveltejs/kit)</summary> ### [`v2.56.1`](https://github.com/sveltejs/kit/blob/HEAD/packages/kit/CHANGELOG.md#2561) [Compare Source](https://github.com/sveltejs/kit/compare/@sveltejs/kit@2.56.0...@sveltejs/kit@2.56.1) ##### Patch Changes - chore: update JSDoc ([#​15640](sveltejs/kit#15640)) ### [`v2.56.0`](https://github.com/sveltejs/kit/blob/HEAD/packages/kit/CHANGELOG.md#2560) [Compare Source](https://github.com/sveltejs/kit/compare/@sveltejs/kit@2.55.0...@sveltejs/kit@2.56.0) ##### Minor Changes - breaking: rework client-driven refreshes ([#​15562](sveltejs/kit#15562)) - breaking: stabilize remote function caching by sorting object keys ([#​15570](sveltejs/kit#15570)) - breaking: add `run()` method to queries, disallow awaiting queries outside render ([#​15533](sveltejs/kit#15533)) - feat: support TypeScript 6.0 ([#​15595](sveltejs/kit#15595)) - breaking: isolate command-triggered query refresh failures per-query ([#​15562](sveltejs/kit#15562)) - feat: use `hydratable` for remote function transport ([#​15533](sveltejs/kit#15533)) - feat: allow `form` fields to specify a default value (`field.as(type, value)`) ([#​15577](sveltejs/kit#15577)) ##### Patch Changes - fix: don't request new data when `.refresh` is called on a query with no cache entry ([#​15533](sveltejs/kit#15533)) - fix: allow using multiple remote functions within one async derived ([#​15561](sveltejs/kit#15561)) - fix: avoid false-positive overridden Vite `base` setting warning when setting a `paths.base` in `svelte.config.js` ([#​15623](sveltejs/kit#15623)) - fix: manage queries in their own `$effect.root` ([#​15533](sveltejs/kit#15533)) - fix: avoid `inlineDynamicImports` deprecation warning when building the service worker with Vite 8 ([#​15550](sveltejs/kit#15550)) - fix: correctly escape backticks when precomputing CSS ([#​15593](sveltejs/kit#15593)) - fix: discard obsolete forks before finishing navigation ([#​15634](sveltejs/kit#15634)) - chore: tighten up override implementation ([#​15562](sveltejs/kit#15562)) - fix: ensure the default Svelte 5 `error.svelte` file uses runes mode ([#​15609](sveltejs/kit#15609)) - fix: deduplicate same-cache-key `batch` calls during SSR ([#​15533](sveltejs/kit#15533)) - fix: decrement pending\_count when form callback doesn't call submit() ([#​15520](sveltejs/kit#15520)) </details> --- ### Configuration 📅 **Schedule**: (in timezone UTC) - Branch creation - At any time (no schedule defined) - Automerge - At any time (no schedule defined) 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My4xMDQuMSIsInVwZGF0ZWRJblZlciI6IjQzLjEwNC4xIiwidGFyZ2V0QnJhbmNoIjoiZGV2ZWxvcCIsImxhYmVscyI6WyJLaW5kL0RlcGVuZGVuY2llcyJdfQ==--> Reviewed-on: https://codeberg.org/ampmod/aw3/pulls/57 Co-authored-by: AmpMod Bot <ampmod-bot@noreply.codeberg.org> Co-committed-by: AmpMod Bot <ampmod-bot@noreply.codeberg.org>
closes #15588
This PR cleans up the escaping logic before we create a function that returns template literal string. Firstly, we have to perform escaping after the CSS processing to avoid interfering with it. Then, only after escaping, can we replace the placeholders with our interpolation placeholders such as
${assets}. This helps avoid those dollar signs from being escaped when they eventually end up in the template literalPlease 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. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits