-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
chore(deps): use vite-plugin-svelte 4 in svelte5 template of create-svelte #12586
Conversation
🦋 Changeset detectedLatest commit: c7d3ea7 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 |
Added svelte5 to create-svelte tests and https://github.com/sveltejs/kit/actions/runs/10439353949/job/28907856687?pr=12586#step:8:111 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!!
Oh, yeah, I guess we'll have to fix that. I might have missed the error in the logs, but all I see is:
It'd be nice if it showed how it failed We could simply revert the changes to |
iirc the logs were too verbose, this is what it logs locally:
|
maybe it's possible to change the code in question so it works without this error? I think it is important that create-svelte creates working projects in all permutations |
Oh yeah. If it's an issue affecting whether it works we'll have to fix it. I thought it was just a style issue |
the error does look like it's a false positive though: $: {
classnames = {};
description = {};
data.answers.forEach((answer, i) => {
const guess = data.guesses[i];
for (let i = 0; i < 5; i += 1) {
// Properties of objects and arrays are not reactive unless in runes mode.
// Changes to this property will not cause the reactive statement to
// update(reactive_declaration_non_reactive_property) eslint(svelte/valid-compile)
const letter = guess[i];
// ^^^^^^^^
if (answer[i] === "x") {
classnames[letter] = "exact";
description[letter] = "correct";
} else if (!classnames[letter]) {
classnames[letter] = answer[i] === "c" ? "close" : "missing";
description[letter] = answer[i] === "c" ? "present" : "absent";
}
}
});
} the project builds and works as expected too edit: the description of the error isn't wrong, but it seems more like it's wrongly presented as an error instead of a warning |
We probably want the whole sverdle refactored to make use of runes as a showcase for svelte5. cc @Rich-Harris |
Was introduced through sveltejs/svelte#12824, I personally think this will cause to many false positives and needs to be tweaked / reverted, but we'll see. Either way it's unrelated to this PR |
this helps to avoid a peer warning during npm install where vite-plugin-svelte@3 still depends on svelte-hmr for svelte4 support, but svelte-hmr does not have a peer range supporting svelte5 (it can't, hmr support is built into svelte5 instead).
Also svelte5 users should use vite-plugin-svelte@4 because that has better support.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits