fix: $destroy and createRoot are no more #328

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

Copy link

Collaborator Author

yanick commented Feb 21, 2024

* `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!

@yanick yanick force-pushed the svelte5-changes branch from 202aefe to 06ba816 Compare

February 22, 2024 16:07

@yanick


 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 yanick force-pushed the svelte5-changes branch from 06ba816 to 44b20b4 Compare

February 22, 2024 16:09

@yanick


 alias pure.js to svelte5.js

d55fbd7

@yanick yanick force-pushed the svelte5-changes branch from 46c2e22 to 2b1449b Compare

February 22, 2024 16:50

@yanick


 fix: test fails for Svelte5 and happy-dom

6f494ad

@yanick 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

@yanick


 context.test.js fails too

914a5bb

@yanick


 add documentation

7ff2e51

@yanick

Copy link

Collaborator Author

yanick commented Feb 22, 2024

@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.

@yanick yanick marked this pull request as ready for review

February 22, 2024 17:58

mcous

mcous requested changes

Feb 22, 2024

View reviewed changes

Copy link

Collaborator

@mcous mcous left a comment

Choose a reason for hiding this comment

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

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?

package.json

"main": "src/index.js",

"exports": {

".": {

"types": "./types/index.d.ts",

Copy link

Collaborator

@mcous mcous Feb 22, 2024

Choose a reason for hiding this comment

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

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

Copy link

Collaborator Author

@yanick yanick Feb 22, 2024

Choose a reason for hiding this comment

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

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

Copy link

Collaborator

@mcous mcous Feb 23, 2024

Choose a reason for hiding this comment

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

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

See #228

package.json Outdated

"default": "./src/index.js"

".": "./src/index.js",

"./svelte5": "./src/svelte5-index.js",

Copy link

Collaborator

@mcous mcous Feb 22, 2024

Choose a reason for hiding this comment

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

Do we need a corresponding types esport for Svelte 5?

src/__tests__/context.test.js Outdated

test('can set a context', () => {

const message = 'Got it'

test.skipIf(SVELTE_VERSION >= '5' && process.env.VITEST_ENV == 'happy-dom')(

Copy link

Collaborator

@mcous mcous Feb 22, 2024

Choose a reason for hiding this comment

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

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?

Copy link

Collaborator Author

@yanick yanick Feb 22, 2024

Choose a reason for hiding this comment

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

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.

src/pure.js Outdated

Comment on lines 13 to 14

if (IS_SVELTE_5)

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

Copy link

Collaborator

@mcous mcous Feb 22, 2024

Choose a reason for hiding this comment

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

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?

Copy link

Collaborator Author

@yanick yanick Feb 22, 2024

Choose a reason for hiding this comment

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

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.

Copy link

Collaborator

@mcous mcous Feb 23, 2024

Choose a reason for hiding this comment

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

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.

Copy link

Collaborator Author

@yanick yanick Feb 23, 2024

Choose a reason for hiding this comment

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

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?

src/svelte5.js

@@ -0,0 +1,145 @@

import {

Copy link

Collaborator

@mcous mcous Feb 22, 2024 •

edited

Loading

Choose a reason for hiding this comment

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

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?

Copy link

Collaborator Author

@yanick yanick Feb 22, 2024

Choose a reason for hiding this comment

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

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.