fix: revert tsconfig change, fix types
#11908
Closing issues
Describe the bug
This is a valid uri for the report-uri directive:
https://123.ingest.sentry.io/api/456/security/?sentry_key=123mykey&sentry_environment=development&sentry_release=sha1-release-hash
But SvelteKit does not approve it's structure. This was noticed after this PR was merged: #11886
See: https://blog.sentry.io/how-sentry-captures-csp-violations/
Reproduction
https://github.com/MathiasWP/sveltekit-csp-report-uri-bug
Logs
No response
System Info
System:
OS: macOS 14.3.1
CPU: (8) arm64 Apple M1 Pro
Memory: 59.19 MB / 16.00 GB
Shell: 5.9 - /bin/zsh
Binaries:
Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm
pnpm: 8.12.0 - /opt/homebrew/bin/pnpm
bun: 1.0.0 - ~/.bun/bin/bun
Browsers:
Brave Browser: 122.1.63.162
Chrome: 121.0.6167.184
Safari: 17.3.1
npmPackages:
@sveltejs/adapter-auto: ^3.0.0 => 3.1.1
@sveltejs/kit: ^2.0.0 => 2.5.2
@sveltejs/vite-plugin-svelte: ^3.0.0 => 3.0.2
svelte: ^4.2.7 => 4.2.12
vite: ^5.0.3 => 5.1.4
Severity
serious, but I can work around it
Additional Information
No response
Describe the bug
After upgrading from kit 2.5.1 to 2.5.2 the follwing svelte.config.js is broken:
import adapter from "@sveltejs/adapter-static";
import { vitePreprocess } from "@sveltejs/vite-plugin-svelte";
/** @type {import('@sveltejs/kit').Config} */
const config = {
preprocess: vitePreprocess(),
kit: {
// ... others
csp: csp(),
},
};
export default config;
function csp() {
if (dev) return;
let directives = {
"default-src": ["something"],
"style-src": ["something", "unsafe-inline"],
"img-src": ["*", "data:"],
"connect-src": ["self"],
"object-src": ["data:"],
"frame-src": ["data:"],
};
return { directives };
}The error I get from typescript is:
Type '{ directives: { 'default-src': string[]; 'style-src': string[]; 'img-src': string[]; 'connect-src': string[]; 'object-src': string[]; 'frame-src': string[]; }; } | undefined' is not assignable to type '{ mode?: "nonce" | "hash" | "auto" | undefined; directives?: CspDirectives | undefined; reportOnly?: CspDirectives | undefined; } | undefined'.
Type '{ directives: { 'default-src': string[]; 'style-src': string[]; 'img-src': string[]; 'connect-src': string[]; 'object-src': string[]; 'frame-src': string[]; }; }' is not assignable to type '{ mode?: "nonce" | "hash" | "auto" | undefined; directives?: CspDirectives | undefined; reportOnly?: CspDirectives | undefined; }'.
The types of 'directives['default-src']' are incompatible between these types.
Type 'string[]' is not assignable to type '(Source | ActionSource)[]'.
Type 'string' is not assignable to type 'Source | ActionSource'.ts(2322)
I don't understand how to fix.
Reproduction
I'll post soon.
Logs
No response
System Info
npmPackages:
@sveltejs/adapter-static: 3.0.1 => 3.0.1
@sveltejs/kit: 2.5.2 => 2.5.2
@sveltejs/vite-plugin-svelte: 3.0.2 => 3.0.2
svelte: 4.2.12 => 4.2.12
Severity
blocking an upgrade
Additional Information
No response
Pull request
The inclusion of svelte.config.js is a breaking change since it's type-checked now and that can break projects which did type-check without errors previously
closes #11906
Also relaxes the report-uri types, fully qualified urls are also ok closes #11905
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
- It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
- This message body should clearly illustrate what problems it solves.
- Ideally, include a test that fails without this PR but passes with it.
Tests
- Run the tests with
pnpm testand lint the project withpnpm lintandpnpm check
Changesets
- If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running
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
- Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.
Info
![changeset-bot[bot]](https://avatars.githubusercontent.com/in/28616?v=4)
🦋 Changeset detected
Latest commit: 16b5ec0
The changes in this PR will be included in the next version bump.
This PR includes changesets to release 1 package
| Name | Type |
|---|---|
| @sveltejs/kit | Patch |
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
The workaround for this bug is pretty nasty. You can't extend include, which means that a user has to put this in their tsconfig.json:
"include": [
"./svelte-kit/ambient.d.ts",
"./svelte-kit/non-ambient.d.ts",
"./svelte-kit/types/**/$types.d.ts",
"svelte.config.js",
"vite.config.js",
"vite.config.ts",
"src/**/*.js",
"src/**/*.ts",
"src/**/*.svelte",
"tests/**/*.js",
"tests/**/*.ts",
"tests/**/*.svelte"
],
I'd be inclined to just fix the CSP types and see if it's still giving folks trouble to include svelte.config.js. While it could be considered a bit of a breaking change to include it, it could also be considered a bug fix as we shouldn't have overlooked it and omitted it in the first place.
I should mention also that the thing that drove #11886 was trying to upgrade a large project to Svelte 5. The project has a ton of createEventDispatcher usage and it turned into a real mess trying to migrate to component props because TypeScript can't differentiate between functions returning void vs Promise<void>. There are, however, several typescript-eslint rules that help enforce proper usage of promises. However, enabling these rules becomes a lot more difficult if #11886 is reverted and I'm afraid that will make it much more of a headache for people to migrate their projects to Svelte 5. Currently the lint rules will only catch issues in the script block. I filed sveltejs/eslint-plugin-svelte#691 to request catching issues in the markup as well.
If the tsconfig change is reverted, perhaps an alternative for now (until the next major version) would be to adjust the create-svelte template to include a // @ts-check at the top of the svelte.config.js file? This change should be OK since it'll only affect new projects.
While that would help the Svelte config file get type checked, it doesn't seem likely to fix the issue I'm trying to address which is that you can't set recommended-type-checked today because the eslint and tsconfig point to different sets of files
The workaround for this bug is pretty nasty. You can't extend
include, which means that a user has to put this in theirtsconfig.json:"include": [ "./svelte-kit/ambient.d.ts", "./svelte-kit/non-ambient.d.ts", "./svelte-kit/types/**/$types.d.ts", "svelte.config.js", "vite.config.js", "vite.config.ts", "src/**/*.js", "src/**/*.ts", "src/**/*.svelte", "tests/**/*.js", "tests/**/*.ts", "tests/**/*.svelte" ],I'd be inclined to just fix the CSP types and see if it's still giving folks trouble to include
svelte.config.js. While it could be considered a bit of a breaking change to include it, it could also be considered a bug fix as we shouldn't have overlooked it and omitted it in the first place.
IMO fixing the CSP rules is a good compromise. I don't have any problems with the svelte config file being type-checked, i only had problems with incorrect types.
Still strongly in favor of reverting this for 2.0 and looking at it again in 3.0
The inclusion of svelte.config.js is a breaking change since it's type-checked now and that can break projects which did type-check without errors previously closes #11906 Also relaxes the report-uri types, fully qualified urls are also ok closes #11905