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.

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

feat(svelte): add support for svelte5 #34715

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

feat(svelte): add support for svelte5 #34715

wants to merge 11 commits into from

Conversation

shfx
Copy link

@shfx shfx commented Feb 10, 2025

Adding support for svelte5.

This change works for us but I need some additional eyes and hands to verify if this is what you might need to upgrade to svelte5. Keep in mind this PR is using somewhat legacy api that is there just to help migration and should be rewritten to "native" before they ditch it (somewhere around svelte6)

Closes #30278

@shfx
Copy link
Author

shfx commented Feb 10, 2025

@microsoft-github-policy-service agree

This comment has been minimized.

This comment has been minimized.

@agg23
Copy link
Contributor

agg23 commented Feb 11, 2025

There's a lot of diff here because of auto-formatting. Please remove these changes.

@@ -18,6 +18,6 @@
"vite": "^5.2.8"
},
"dependencies": {
"svelte": "^4.2.8"
"svelte": "^5.19.9"
Copy link
Member

Choose a reason for hiding this comment

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

I think we actually want to introduce ct-svelte4-vite to keep testing for vite4, wdyt @agg23

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll will try to do that today. Thanks for the input.

@shfx shfx marked this pull request as draft February 13, 2025 10:18
@shfx shfx force-pushed the main branch 2 times, most recently from bc06d07 to 0d076a0 Compare February 15, 2025 16:18

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

@agg23
Copy link
Contributor

agg23 commented Feb 18, 2025

No rush, but you still have a bunch of formatting changes and stuff that doesn't pass the linter.

@shfx shfx force-pushed the main branch 2 times, most recently from 7942ab7 to ee03658 Compare February 18, 2025 18:54
@shfx
Copy link
Author

shfx commented Feb 18, 2025

Ugh, some issues after a rebase. I will fix those issues tomorrow, both linting and npm issue as I don't have much time after my 9-5 job.

I also don't think I can fully make it backward compatible as there is no option to attach slots to svelte4 components like there was in the previous version of the library.

@shfx
Copy link
Author

shfx commented Feb 18, 2025

I think bumping vite to 6.1.0 in the project root broke my pipelines.

@shfx
Copy link
Author

shfx commented Feb 18, 2025

Will try to rebase tomorrow if there is a fix for that.

@shfx shfx force-pushed the main branch 2 times, most recently from f051d3c to bbd4642 Compare February 20, 2025 22:55
@shfx
Copy link
Author

shfx commented Feb 20, 2025

I don't think I can fix the pipeline without someone bumping dependencies for react

❯ npm i        
npm error code ERESOLVE
npm error ERESOLVE could not resolve
npm error
npm error While resolving: @vitejs/[email protected]
npm error Found: [email protected]
npm error node_modules/vite
npm error   dev vite@"^6.1.0" from the root project
npm error   peer vite@"^3.0.0 || ^4.0.0 || ^5.0.0 || ^6.0.0" from @vitejs/[email protected]
npm error   node_modules/@vitejs/plugin-basic-ssl
npm error     dev @vitejs/plugin-basic-ssl@"^1.2.0" from the root project
npm error   4 more (@sveltejs/vite-plugin-svelte, ...)
npm error
npm error Could not resolve dependency:
npm error peer vite@"^4.2.0 || ^5.0.0" from @vitejs/[email protected]
npm error node_modules/@vitejs/plugin-react
npm error   dev @vitejs/plugin-react@"^4.2.1" from the root project
npm error   @vitejs/plugin-react@"^4.2.1" from @playwright/[email protected]
npm error   packages/playwright-ct-react
npm error     @playwright/[email protected]
npm error     node_modules/@playwright/experimental-ct-react
npm error       workspace packages/playwright-ct-react from the root project
npm error   1 more (@playwright/experimental-ct-react17)
npm error
npm error Conflicting peer dependency: [email protected]
npm error node_modules/vite
npm error   peer vite@"^4.2.0 || ^5.0.0" from @vitejs/[email protected]
npm error   node_modules/@vitejs/plugin-react
npm error     dev @vitejs/plugin-react@"^4.2.1" from the root project
npm error     @vitejs/plugin-react@"^4.2.1" from @playwright/[email protected]
npm error     packages/playwright-ct-react
npm error       @playwright/[email protected]
npm error       node_modules/@playwright/experimental-ct-react
npm error         workspace packages/playwright-ct-react from the root project
npm error     1 more (@playwright/experimental-ct-react17)
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

@agg23
Copy link
Contributor

agg23 commented Feb 21, 2025

I think you should be able to update @vitejs/plugin-react without issue.

@shfx
Copy link
Author

shfx commented Feb 21, 2025

I can, was not sure if I should do it in this PR. Gonna try it today.

This comment has been minimized.

Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › tests/reporter-html.spec.ts:2737:5 › merged › execSync doesnt produce a second stdout attachment @macos-latest-node18-2

2834 passed, 13 skipped
✔️✔️✔️

Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: experimental-ct-svelte Svelte 5 support - svelte/internal won't be available
3 participants