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

breaking: remove support for svelte 4 and support svelte 5 #30700

Merged
merged 1 commit into from
Dec 10, 2024

Conversation

AtofStryker
Copy link
Contributor

@AtofStryker AtofStryker commented Dec 3, 2024

Additional details

Implements Svelte 5 Component Testing for Cypress. Since Svelte 5 had a lot of changes to how components are mounted (see migration guide) as well as types, we felt it was best to support Svelte 5 moving forward and removing support for Svelte 4. The Svelte 4 test harness can always be downloaded from npm, which is explicitly called out in the migration guide.

There are some changes to the Svelte API with Svelte 5 which I try to cover in comments below.

NOTE: The semantic PR job is going to fail because we are referencing one issue as a feature, and the other as a breaking change. I do not think there is a way to get this to pass here

Steps to test

I recommend using the build binaries on the commit to test against a vite project via npm create vite@latest or testing via cypress-io/cypress-component-testing-apps#40

How has the user experience changed?

Users can now scaffold their Svelte 5 projects with Cypress and component testing will work as expected

Screenshot 2024-12-05 at 9 36 28 AM Screenshot 2024-12-05 at 9 36 32 AM Screenshot 2024-12-05 at 9 39 29 AM

PR Tasks

Copy link

cypress bot commented Dec 3, 2024

cypress    Run #58762

Run Properties:  status check passed Passed #58762  •  git commit 2043d855d6: breaking: remove support for svelte 4 and support svelte 5 [run ci]
Project cypress
Branch Review feat/support_svelte_5
Run status status check passed Passed #58762
Run duration 17m 53s
Commit git commit 2043d855d6: breaking: remove support for svelte 4 and support svelte 5 [run ci]
Committer AtofStryker
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 8
Tests that did not run due to a developer annotating a test with .skip  Pending 1326
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 29383
View all changes introduced in this branch ↗︎
UI Coverage  46.2%
  Untested elements 186  
  Tested elements 164  
Accessibility  92.55%
  Failed rules  3 critical   8 serious   2 moderate   2 minor
  Failed elements 893  

@AtofStryker AtofStryker force-pushed the feat/support_svelte_5 branch 13 times, most recently from 64a9974 to fe87cde Compare December 4, 2024 03:58
@AtofStryker AtofStryker linked an issue Dec 4, 2024 that may be closed by this pull request
cli/CHANGELOG.md Outdated Show resolved Hide resolved
@@ -1,4 +1,10 @@
import { createEntries } from '@cypress/mount-utils/create-rollup-entry.mjs'

const config = {
external: [
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we did not need svelte as an imported dependency before, but now we need the mount and unmount functions in svelte 5. Since we want to use whatever version of svelte the user has installed, we want to bundle this as an external dependency

options: MountOptions<T> = {},
): Cypress.Chainable<MountReturn<T>> {
export function mount (
Component: Component<Record<string, any>, Record<string, any>, any>,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The component signature for svelte completely changed in Svelte 5, where Svelte components are no longer generic classes . Instead, the typing represents something more simple now, which is just a generic type of Record<string, any>

): Cypress.Chainable<MountReturn> {
// In Svelte 5, the component name is no longer easily discoverable and logs as "wrapper"
// so we default the logging of it to false as it doesn't provide a lot of value
options.log = options.log || false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

depending on how Svelte is bundled has an impact here, where the name of the component can only be determined to be wrapper. For instance, the name in open mode is wrapper, but in run mode it's the full component name. This like has to do with the development version of svelte being used in open mode but production being used in run.

Because of this, I have default the mount log off by default as when developing tests it doesn't provide a lot of value to just say mount<wrapper>

npm/svelte/src/mount.ts Outdated Show resolved Hide resolved
| Next.js | 14.x | Webpack | 4.x, 5.x | React 18 | `@cypress/react@latest` | [Link](../../system-tests/projects/nextjs-configured) |
| Name | Version| Dev Server | Version | Library | Component Adaptor | Example Project |
| ---------------- | -------| ---------- | ------- | ------------------ | -------------------------- | ------------------------------------------------------------------- |
| React | - | Vite | 4, 5 | React 18, 19 | `@cypress/react@latest` | [Link](../../system-tests/projects/react-vite-ts-configured) |
Copy link
Contributor Author

Choose a reason for hiding this comment

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

figured this was a good time to make sure the list is up to date

@@ -1,10 +1,10 @@
<script>
let count = 0
<script lang="ts">
Copy link
Contributor Author

@AtofStryker AtofStryker Dec 5, 2024

Choose a reason for hiding this comment

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

most of the changes to these components are to follow the svelte migration guide

biggest differences:

  • similar to React now, is that Svelte components need to explicitly declare state or props for a given property. This is why there is Counter.svelte which uses state and CounterProp.svelte which uses a prop to set the counter.
  • Events are NOT propagated up to components by default and event dispatchers are deprecated. The recommended pattern is now to pass a callback function in as a prop and call that when the event is fired (see Changes to Component Events)

There are a few others but these are the main changes. I also converted the projects to typescript as this is the new base case

loader: 'svelte-loader',
test: /\.svelte\.ts$/,
use: ['svelte-loader', 'ts-loader'],
},
Copy link
Contributor Author

@AtofStryker AtofStryker Dec 5, 2024

Choose a reason for hiding this comment

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

this is copied straight out of svelte-loader README for simplicity

// svelte-webpack-configured is currently difficult to test.
// This is currently tested as a binary-like system-test inside CI.
// We can unskip this test and remove the binary-like CircleCI test
// once https://github.com/sveltejs/svelte-loader/issues/243 is resolved.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see the readme inside svelte-webpack-configured for why we need to do this. Main reason, aliasing svelte inside webpack doesn't work correctly, so the mount function being pulled in from npm/svelte is a different svelte instance than the one in svelte-webpack-configured. The work around for this is to use the binary (see circle workflow file) as this can't happen when the binary is used instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I get it more then. So this won't be an issue for our users right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

correct this is just a problem with our system tests because of how we source the harnesses straight from the ./cli package. installing the binary sidesteps this problem

@AtofStryker AtofStryker self-assigned this Dec 5, 2024
@AtofStryker AtofStryker force-pushed the feat/support_svelte_5 branch 2 times, most recently from b97fba5 to 45d2f7d Compare December 5, 2024 15:08
@AtofStryker AtofStryker marked this pull request as ready for review December 5, 2024 15:09
@AtofStryker AtofStryker force-pushed the feat/support_svelte_5 branch from 09394f5 to 822ba62 Compare December 10, 2024 13:01
@AtofStryker AtofStryker force-pushed the feat/support_svelte_5 branch from 822ba62 to 2043d85 Compare December 10, 2024 13:44
@AtofStryker AtofStryker merged commit 22776eb into release/14.0.0 Dec 10, 2024
80 of 83 checks passed
@AtofStryker AtofStryker deleted the feat/support_svelte_5 branch December 10, 2024 14:21
@AtofStryker AtofStryker linked an issue Dec 10, 2024 that may be closed by this pull request
@jennifer-shehane jennifer-shehane mentioned this pull request Dec 11, 2024
28 tasks
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.

breaking: remove support for Svelte 4 Svelte 5 component testing
2 participants