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: add Svelte v5-next support #321

Merged
merged 5 commits into from
Feb 13, 2024

Conversation

mcous
Copy link
Collaborator

@mcous mcous commented Feb 12, 2024

Overview

This PR is an alternative approach to #284 (CC @yanick) that adds experimental support for svelte@next without breaking support for Svelte v3 nor v4. I think adding support for the prerelease version of Svelte v5 as soon as possible will be a really helpful move for everyone trying to migrate - myself very much included.

This PR requires the cleanup work I did in #314. Please see mcous#1 in my fork for the incremental diff from #314 to here. This PR is ready for review, but #314 should merge first

Issues encountered

Work after this PR

  • If we want to guarantee that onMount and onDestroy are called, we'll need to make render async so it can call tick() after render, which will be a breaking change
  • Accordingly, I think we'll still want to issue a major bump that drops Svelte 3 and Node 16, maybe when Svelte 5 proper is released
  • We can and should keep Svelte 4 support, in my opinion

@RobinKnipe
Copy link

Is there any consumer emulation as part of the CI flow?
I've setup a svelte5 project (with npx create svelte@latest) and chosen vitest support, then changed to this PR's commit:

 + @testing-library/svelte@git+ssh://github.com/mcous/svelte-testing-library#150335aac28f071710ca5771d4dc2a4f7363633c

 1 package installed [1.56s]

...but running a simple component test still fails to render 😞 I had hoped for more success seeing all the jsdom checks passing here, but I'm still seeing the same error as the latest release:

Instantiating a component with `new` is no longer valid in Svelte 5.

@yanick
Copy link
Collaborator

yanick commented Feb 12, 2024

It'll soon be in a next release. I've just cut the very first testing of next this morning with some other changes, and I'll try to add in the v5 support in that branch later on. Hold tight, I'll try to report on it before the end of the day.

@mcous
Copy link
Collaborator Author

mcous commented Feb 12, 2024

@RobinKnipe that smells like a dependency install issue, you might want to check if you have duplicate versions installed with npm why @testing-library/svelte (if I'm remembering that command properly).

Like @yanick said, you'll probably have a happier time if you wait for the prerelease version in npm. I've never messed around with installing this library via git so I don't really know what pitfalls and limitations exist, especially coming if it's coming from a fork rather than upstream

If you're curious what our CI does, check out release.yml in the workflows folder; it's fairly straightforward.

@RobinKnipe
Copy link

RobinKnipe commented Feb 13, 2024

If it helps; some partial success in this test project. Noddy unit tests created for the 2 components from the default "create svelte" setup (one working, one failing).

This is using the changes in this PR:

svelte5$ npm why @testing-library/svelte
@testing-library/svelte@0.0.0-semantically-released dev
node_modules/@testing-library/svelte
  dev @testing-library/svelte@"git+ssh://github.com/mcous/svelte-testing-library#150335aac28f071710ca5771d4dc2a4f7363633c" from the root project

@mcous
Copy link
Collaborator Author

mcous commented Feb 13, 2024

@RobinKnipe your example repo has three problems, all interacting/hiding each other, that are causing that <Header> failure. All three issues are about how to test SvelteKit apps, not Svelte v5. The exact same failure happens if you switch your repo to Svelte 4.

  • Vite is configured with the vanilla Svelte plugin rather than SvelteKit, which hides...
  • Vite is not configured to resolve the browser bundle of Svelte, which hides...
  • The SvelteKit $page store will be empty in unit tests, so your test crashes on:
    TypeError: Cannot read properties of undefined (reading 'pathname')
     ❯ Object.hydrate [as h] src/routes/Header.svelte:19:32
         17|   </svg>
         18|   <ul>
         19|    <li aria-current={$page.url.pathname === '/' ? 'page' : undefined}>
           |                                ^
         20|     <a href="/">Home</a>
         21|    </li>
    

Since these are not issues with this library nor Svelte v5, if you have any additional questions, please open a separate discussion if you need help testing a SvelteKit app

@yanick yanick changed the base branch from main to next February 13, 2024 17:21
@yanick
Copy link
Collaborator

yanick commented Feb 13, 2024

Basing on 'next', and I'm going to push the conflict resolutions in 2 minutes.

@yanick
Copy link
Collaborator

yanick commented Feb 13, 2024

@mcous You good with me merging this bad boy into next and seeing if it triggers our first next-type release to npm?

@mcous
Copy link
Collaborator Author

mcous commented Feb 13, 2024

@yanick oh boy 😬, no time like the present

The only thing giving me pause is that our next branch is not configured as a prerelease in release.yml. So I think merging may bump to 5.0.0 proper and release it on the next tag.

Not sure what your desires are on 5.0.0 vs 5.0.0-next.0 (or whatever). Either way I'm happy with what you choose! I'm fairly confident with this PR given the test matrix and the low risk to harming Svelte v4 support

@yanick
Copy link
Collaborator

yanick commented Feb 13, 2024

The only thing giving me pause is that our next branch is not configured as a prerelease in release.yml. So I think merging may bump to 5.0.0 proper and release it on the next tag.

Oh yeah, you're right. next and main are regular releases, and alpha and beta branches are prereleases.

Not sure what your desires are on 5.0.0 vs 5.0.0-next.0 (or whatever). Either way I'm happy with what you choose! I'm fairly confident with this PR given the test matrix and the low risk to harming Svelte v4 support

Hmmm, it's true that with the test suite, the chances that we'll utterly bugger up everything are slim (and yes, I just jinxed us by saying that). But today is a beautiful day to di-- I mean, deploy, so let's Leeroy Jenkins this puppy out and see what happens. >.>

@yanick yanick marked this pull request as ready for review February 13, 2024 18:10
@yanick yanick merged commit 178b2de into testing-library:next Feb 13, 2024
15 of 16 checks passed
Copy link

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mcous
Copy link
Collaborator Author

mcous commented Feb 13, 2024

Hey, cool! Looks like it missed the breaking fix! for the rerender changes, but I'd like to land a few more pedantically breaking changes into next anyway (see #312 and #313) so we'll see what happens

@mcous mcous deleted the add-svelte-5-non-breaking branch February 13, 2024 19:46
@RobinKnipe
Copy link

@mcous ah it's SvelteKit related!? Ok, I'll try and refine my testing to Svelte only examples. Thanks!

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

Successfully merging this pull request may close these issues.

None yet

3 participants