fix: $destroy and createRoot are no more #328

yanick · 2024-02-20T17:39:59Z

No description provided.

mcous

Feel free to ping me once this is in and I'll rebase #325 accordingly

yanick · 2024-02-21T14:07:13Z

Feel free to ping me once this is in and I'll rebase #325 accordingly

I will! I'm still figuring out how to deal with $set. I'll push the code I have right now for shared lulz. It's not yet working, though.

mcous · 2024-02-21T14:27:19Z

I added the mount / unmount changes to #325 before you made this PR, which will at least unblock the majority of testing. I'm starting to think that either:

rerender won't be supported in Svelte 5
- Instead, the complexity is pushed to the user, who will need to do something like
```
// comp.test.svelte.js
const props = $state({ name: 'hello' })
render(Comp, { props })
// ...
act(() => (props.name = 'goodbye'))
```
- This is how solid-testing-library operates
We'll need to introduce a build step into this library so we can use $state ourselves
- I actually don't know if this will work

Either path feels like a bit of a bummer!

yanick · 2024-02-21T14:45:53Z

* `rerender` won't be supported in Svelte 5
  
  * Instead, the complexity is pushed to the user, who will need to do something like
    ```js
    // comp.test.svelte.js
    const props = $state({ name: 'hello' })
    render(Comp, { props })
    // ...
    act(() => (props.name = 'goodbye'))
    ```
  * This is how `solid-testing-library` operates

Yup, that's what I think as well. I'm trying to change the rerender test to use that ^^^ pattern, but for some reason it doesn't react to props.name = 'goodbye'. But I used a waitFor, not an act, so I'll try that.

* We'll need to introduce a build step into this library so we can use `$state` ourselves

I think we just need to change the name of the test file to be test.svelte.js for the compiler to do the right thing. I'll say this with more conviction once the test is passing. :-)

Right now my battle plan is

find out the Way That Works with Svelte 5.
see how that can fit with the previous versions of svelte. Worst case we might have to split things and have import {} from '@testing-library/svelte for the latest supported version and import {} from '@testing-library/svelte/svelteX for X that are legacy versions and upcoming versions.
Depending on (2), organize the tests to cover the maximum without going insane.

So yeah, weeeeee, supporting several major versions is fun, fun, fun. 🤪

  * I actually don't know if this will work
Either path feels like a bit of a bummer!

For now I've resorted to use the legacy API, as the use of runes don't seem to work in the test environment (which, mind you, could be a problem on this side of the keyboard) and the important part is to have the package work with Svelte 5.

yanick · 2024-02-22T17:58:33Z

@mcous This is not the patch we deserve, but probably the patch we need. I'm using the svelte/legacy API of svelte 5 as I could not make the runes work in the testing context yet. I've also skipped 2 tests for svelte 5 and happy-dom because they are failing in a weird way.

mcous

Requesting changes because this breaks type exports in package.json for the existing library.

Additionally, the separate entry points for v4 vs v5 seems a little unnecessary from a technical standpoint, because the public APIs remain unchanged. What's the value of it this approach over what's currently in #325, where pure.js continues to support both?

mcous · 2024-02-22T18:57:21Z

package.json

@@ -4,10 +4,8 @@
  "description": "Simple and complete Svelte testing utilities that encourage good testing practices.",
  "main": "src/index.js",
  "exports": {
-    ".": {
-      "types": "./types/index.d.ts",


Oops, we need to make sure we don't drop this types export, or it'll break TS using more modern moduleResolution modes

Doesn't the types key right below the exports take care of that, though?

Unfortunately, no. If TS sees "exports" in moduleResolute: nodenext or bundler and there's no exports[].types it'll get sad

See #228

mcous · 2024-02-22T18:57:41Z

package.json

-      "default": "./src/index.js"
-    },
+    ".": "./src/index.js",
+    "./svelte5": "./src/svelte5-index.js",


Do we need a corresponding types esport for Svelte 5?

mcous · 2024-02-22T18:59:55Z

src/__tests__/context.test.js


 import { render } from '..'
 import Comp from './fixtures/Context.svelte'

-test('can set a context', () => {
-  const message = 'Got it'
+test.skipIf(SVELTE_VERSION >= '5' && process.env.VITEST_ENV == 'happy-dom')(


In another test I used the user-agent to test for jsdom vs happy-dom. It's ever-so-slightly janky, but I like it because it doesn't require adding more environment variables / setup. Using an environment variable makes it harder to use vitest directly, which I do with some frequency to noodle on stuff

Related, should we have a little __tests__/environment.ts helper file that exports IS_SVELTE_5, IS_HAPPY_DOM, IS_JSDOM, etc. to make these a little less repetitive?

should we have a little tests/environment.ts helper file that exports IS_SVELTE_5, IS_HAPPY_DOM, IS_JSDOM, etc. to make these a little less repetitive?

Yeah, that would make a lot of sense.

mcous · 2024-02-22T19:04:52Z

src/pure.js

+if (IS_SVELTE_5)
+  console.warn('for Svelte 5, use `@testing-library/svelte/svelte5`')


In general, I'm open to the separate entry points approach! That being said, I don't see anything in this PR that really justifies it. Is it a future-proofing thing?

As mentioned in the previous comment, it's to deal with import from 'svelte/legacy'. We could probably dig out darker magic to do it another way, but I do feel there will be other things in Svelte 5 that will be easier if we just have different files implementing the same(ish) interface rather than having a file that tries to satisfy all versions at once.

Doh, yeah duh, my bad, that makes total sense.

Aside: this makes me think dropping 3.0 in the next branch (in a separate PR to be responsible about it) will make things easier.

I'm not totally sure what to do about collecting multiple breaking changes without a prerelease version. Maybe we should switch over to an beta branch for breaking changes? Or consider switching next to be a prerelease in semantic-release.

Aside: this makes me think dropping 3.0 in the next branch (in a separate PR to be responsible about it) will make things easier.

What would we cut off?

mcous · 2024-02-22T19:05:50Z

src/svelte5.js

@@ -0,0 +1,145 @@
+import {


This feels like a lot of duplication for little payoff. If you want to have separate entry points, would it be better to re-export pure.js's exports and only override the public methods that changed?

Alternatively, can we move the split to an internal facade instead?

My development steps are

Make it

Make it work

Make it pretty

This PR is at stage (2). all the functions that aren't different in svelte5.js should really be imported from pure.js. But first I was interested to see how deep the hole had to be. And then backfill and tidy up the garden again.

Sure thing, happy to defer to whatever approach you want to take. Just wanted to point out opportunities nip stuff in the bud LoC-wise

yanick · 2024-02-22T21:10:15Z

What's the value of it this approach over what's currently in #325, where pure.js continues to support both?

import {...} from 'svelte/legacy' goes boom for Svelte 3 that didn't have that package.

github-actions · 2024-02-24T16:56:10Z

🎉 This PR is included in version 4.2.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

mcous approved these changes Feb 20, 2024

View reviewed changes

yanick force-pushed the svelte5-changes branch from 202aefe to 06ba816 Compare February 22, 2024 16:07

fix: make the latest Svelte 5 pass all tests

44b20b4

For now I've resorted to use the legacy API, as the use of runes don't seem to work in the test environment (which, mind you, could be a problem on this side of the keyboard) and the important part is to have the package work with Svelte 5.

yanick force-pushed the svelte5-changes branch from 06ba816 to 44b20b4 Compare February 22, 2024 16:09

alias pure.js to svelte5.js

d55fbd7

yanick force-pushed the svelte5-changes branch from 46c2e22 to 2b1449b Compare February 22, 2024 16:50

fix: test fails for Svelte5 and happy-dom

6f494ad

yanick force-pushed the svelte5-changes branch from 2b1449b to 6f494ad Compare February 22, 2024 16:53

yanick added 2 commits February 22, 2024 12:02

context.test.js fails too

914a5bb

add documentation

7ff2e51

yanick marked this pull request as ready for review February 22, 2024 17:58

mcous requested changes Feb 22, 2024

View reviewed changes

yanick added 4 commits February 23, 2024 12:37

de-duplicate code

9d86208

tweak the documentation

92b651f

put back the types in export

8f233ab

test util file

a3d1c96

yanick merged commit 40973c5 into next Feb 24, 2024
16 checks passed

github-actions bot added the released on @next label Feb 24, 2024

mcous mentioned this pull request Mar 17, 2024

fix: ensure fireEvent is exported #339

Merged

mcous mentioned this pull request May 14, 2024

test: fix auto-cleanup tests #371

Merged

mcous deleted the svelte5-changes branch June 2, 2024 17:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: $destroy and createRoot are no more #328

fix: $destroy and createRoot are no more #328

yanick commented Feb 20, 2024

mcous left a comment

yanick commented Feb 21, 2024

mcous commented Feb 21, 2024

yanick commented Feb 21, 2024

yanick commented Feb 22, 2024

mcous left a comment

mcous Feb 22, 2024

yanick Feb 22, 2024

mcous Feb 23, 2024

mcous Feb 22, 2024

mcous Feb 22, 2024

yanick Feb 22, 2024

mcous Feb 22, 2024

yanick Feb 22, 2024

mcous Feb 23, 2024

yanick Feb 23, 2024

mcous Feb 22, 2024 •

edited

Loading

yanick Feb 22, 2024

mcous Feb 23, 2024

yanick commented Feb 22, 2024

github-actions bot commented Feb 24, 2024

		if (IS_SVELTE_5)
		console.warn('for Svelte 5, use `@testing-library/svelte/svelte5`')

fix: $destroy and createRoot are no more #328

fix: $destroy and createRoot are no more #328

Conversation

yanick commented Feb 20, 2024

mcous left a comment

Choose a reason for hiding this comment

yanick commented Feb 21, 2024

mcous commented Feb 21, 2024

yanick commented Feb 21, 2024

yanick commented Feb 22, 2024

mcous left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous Feb 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanick commented Feb 22, 2024

github-actions bot commented Feb 24, 2024

mcous Feb 22, 2024 •

edited

Loading