Skip to content
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

Jump to bottom

feat: upgrade to svelte 5 #35

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

feat: upgrade to svelte 5 #35

wants to merge 11 commits into from

Conversation

bobbymannino
Copy link
Contributor

  • upgrade to svelte 5
  • colors can now be 3 hex digits or 6 ie #ffe or #00ffee
  • if settings fails to save because invalid data, it will alert the user what data is invalid ie on username invalid

gorillamoe
gorillamoe reviewed Dec 3, 2024
View reviewed changes
Copy link
Member

@gorillamoe gorillamoe left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your continuous effort!

Can you have a look at the conflicts?

@gorillamoe gorillamoe linked an issue Dec 3, 2024 that may be closed by this pull request
Feaature Request: Svelte 5 #33
Open
@bobbymannino
Copy link
Contributor Author

once youve merged it open it run, run it, build it, it works on my machine but i havent ran it through any proper tests

@gorillamoe
Copy link
Member

gorillamoe commented Dec 3, 2024 •
edited
Loading

I get this error on a clean clone:

❯ git st
On branch feat/upgrade-to-svelte-5
nothing to commit, working tree clean
❯ vim package.json
❯ npm ci
npm error code ERESOLVE
npm error ERESOLVE could not resolve
npm error
npm error While resolving: @sveltejs/[email protected]
npm error Found: @sveltejs/[email protected]
npm error node_modules/@sveltejs/vite-plugin-svelte
npm error   dev @sveltejs/vite-plugin-svelte@"^5.0.0" from the root project
npm error   peer @sveltejs/vite-plugin-svelte@"^5.0.0" from @sveltejs/[email protected]
npm error   node_modules/@sveltejs/vite-plugin-svelte-inspector
npm error     @sveltejs/vite-plugin-svelte-inspector@"^4.0.0" from @sveltejs/[email protected]
npm error
npm error Could not resolve dependency:
npm error peer @sveltejs/vite-plugin-svelte@"^3.0.0 || ^4.0.0-next.1" from @sveltejs/[email protected]
npm error node_modules/@sveltejs/kit
npm error   peer @sveltejs/kit@"^2.4.0" from @sveltejs/[email protected]
npm error   node_modules/@sveltejs/adapter-node
npm error     dev @sveltejs/adapter-node@"^5.2.9" from the root project
npm error
npm error Conflicting peer dependency: @sveltejs/[email protected]
npm error node_modules/@sveltejs/vite-plugin-svelte
npm error   peer @sveltejs/vite-plugin-svelte@"^3.0.0 || ^4.0.0-next.1" from @sveltejs/[email protected]
npm error   node_modules/@sveltejs/kit
npm error     peer @sveltejs/kit@"^2.4.0" from @sveltejs/[email protected]
npm error     node_modules/@sveltejs/adapter-node
npm error       dev @sveltejs/adapter-node@"^5.2.9" from the root project
npm error
npm error Fix the upstream dependency conflict, or retry
npm error this command with --force or --legacy-peer-deps
npm error to accept an incorrect (and potentially broken) dependency resolution.
npm error
npm error
npm error For a full report see:
npm error /home/marco/.npm/_logs/2024-12-03T12_38_52_184Z-eresolve-report.txt

npm error A complete log of this run can be found in: /home/marco/.npm/_logs/2024-12-03T12_38_52_184Z-debug-0.log

that is exaclty what I also faced, that the peer deps couldn't be satisfied, so at some point I was like "fuck it", let's just keep svelte as is and work on features 🙈

so you can just clean clone the repo and run that or try to remove the node_modules folder and run npm ci

@bobbymannino
Copy link
Contributor Author

bobbymannino commented Dec 3, 2024 •
edited
Loading

yes sorry forgot to mention, vite v6 came out a couple days ago or something and its not yet "officialy" supported by electron, it does work but with npm you have to pass in -f flag like npm i -f to force install deps

Edit: i think for npm ci passing the same flag should work too

@bobbymannino
Copy link
Contributor Author

should also close #40 with this one

@gorillamoe
Copy link
Member

yes sorry forgot to mention, vite v6 came out a couple days ago or something and its not yet "officialy" supported by electron, it does work but with npm you have to pass in -f flag like npm i -f to force install deps

Edit: i think for npm ci passing the same flag should work too

Ye, with passing the --force flag it also bypasses like every peer-deps check, which then renders ci obsolete 😢

This also breaks the current CI/CD implementation. Let's keep this one open as it's good work, but I won't merge it as long as it breaks CI/CD and adding --force to the CI/CD is not something I want to do.

There is a reason we use npm ci in the context of the CI to make builds reliable.

@gorillamoe gorillamoe marked this pull request as draft December 3, 2024 13:21
@gorillamoe gorillamoe added the enhancement New feature or request label Dec 3, 2024
@bobbymannino
Copy link
Contributor Author

completely valid, ill keep an eye out for when it becomes properly supported and add it then :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gorillamoe gorillamoe gorillamoe left review comments

Assignees

@bobbymannino bobbymannino

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feaature Request: Svelte 5
2 participants
@bobbymannino @gorillamoe