-
Notifications
You must be signed in to change notification settings - Fork 33
fix(svelte5): use state rune for rerender #374
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
Conversation
const instance = new Svelte5TestingLibrary() | ||
|
||
export const render = instance.render.bind(instance) | ||
export const cleanup = instance.cleanup.bind(instance) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yanick I find this singleton-class-instance-for-deduping-via-inheritance a bit confusing. I think this could be implemented more cleanly and directly via more procedural function composition. Would you be open to such a change, especially to merge svelte5
back into pure
for the Svelte 5 production release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends a lot what that "more cleanly and directly" looks like. For what it's worth, when svelte5 will become the default, I was not going to merge it back into pure
but rather have the default point to it, so that
import {} from 'svelte-testing-library' ;
gives you the official live svelte version of the day.
import {} from 'svelte-testing-library/svelte4' ;
gives you the svelte 4 version
import {} from 'svelte-testing-library/svelte5' ;
gives you the svelte 5 version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It depends a lot what that "more cleanly and directly" looks like
Given the small scope of this library, I think something that more obviously (and, to be honest, crudely) maps out the branches as you read would be easier to follow as a reader:
// pure.js
export const render = (...) => {
// ...
const component = IS_SVELTE_5
? renderSvelte5Component(...)
: renderLegacyComponent(...)
// ...
}
export const cleanup = () => {
componentCache.forEach(cleanupComponent)
// ...
}
const cleanupComponent = (component) => {
if (IS_SVELTE_5) {
cleanupSvelte5Component(component)
} else {
cleanupLegacyComponent(component)
}
}
I was not going to merge it back into pure but rather have the default point to it, so that
import {} from 'svelte-testing-library' ; gives you the official live svelte version of the day.
import {} from 'svelte-testing-library/svelte4' ; gives you the svelte 4 version
import {} from 'svelte-testing-library/svelte5' ; gives you the svelte 5 version
Have you read through the inbound issues on #284? The separate entry points have been leading to relatively frequent cleanup issues and general confusion. Splitting the library into separate singleton instances with independent state has been a little rough. Plus it's been hard to get the APIs correct on our end as authors: see #346, #339.
I don't think it's a pattern we should keep going with. I don't see any technical reason to continue with separate entry points given that the svelte/legacy
import is no longer needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export const render = (...) => {
// ...
const component = IS_SVELTE_5
? renderSvelte5Component(...)
: renderLegacyComponent(...)
// ...
}
My original reaction is 'oh sweet baby jesus no'. Readability is often in the eye in the beholder, and mushing things together like that is pretty much my definition of how not to do it. That doesn't get my vote, sorry.
. I don't see any technical reason to continue with separate entry points given that the svelte/legacy import is no longer needed
We might have different expectations there. Even when svelte5 is release, I expect the testing library to still support svelte 4 as long as it doesn't get too onerous to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My original reaction is 'oh sweet baby jesus no'. Readability is often in the eye in the beholder, and mushing things together like that is pretty much my definition of how not to do it. That doesn't get my vote, sorry.
My example was intentionally crude for the purposes of a GitHub comment. What I'm proposing is continuing to use the breakdown of tasks that the classes use, but simply export functions instead of using singletons and bind
s everywhere. There are real technical downsides and footguns to the class-based approach that we've been dealing with.
Regardless, I'd like us to come up with a solution that doesn't split the cleanup cache up, which seems to be the main driver of issues that users have been encountering.
I don't see any technical reason to continue with separate entry points given that the svelte/legacy import is no longer needed
We might have different expectations there. Even when svelte5 is release, I expect the testing library to still support svelte 4 as long as it doesn't get too onerous to do it.
We have the same expectations, this library should continue to support the versions of Svelte it works with today. What I'm saying is that this library doesn't need separate entry-points to do so. A single entry-point can easily support Svelte 3, 4 and 5
As a user of this library, I find the separate entry-points to be a weird bit of busy work to force on me as I upgrade. I think as library authors we should strive to force as little upgrade work on our users as possible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Threw up a little WIP PR for what's on my mind re: no singletons + single entrypoint:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My example was intentionally crude for the purposes of a GitHub comment.
Then you can understand how it doesn't do a super job of selling your point.
Now, I took maintainership of this project because it was needed and, well, because it was fun. But there are a lot of other balls I have to juggle outside of this project, and I'm afraid you're pulling too fast and too much in a different direction for this old man to keep up. So it makes sense for me to take a step back and a little sabbatical. You already have the keys to the car. So go ahead and do what thou wilt. And best of luck. :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha! I appreciate you keeping the project going. I'll keep an eye out for any feedback whenever you feel like dropping by
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this file to svelte5.svelte.js
so the Svelte compiler knows to pick it up and process runes. Unfortunately, the rename seems to have confused the git history
svelteComponentOptions = [ | ||
'target', | ||
'props', | ||
'events', | ||
'context', | ||
'intro', | ||
'recover', | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm I don't think this list is up to date with the latest Svelte 5; probably worth a double-check outside of this PR. It looks like it's the list of options for hydrate
rather than mount
- Missing
anchor
recover
doesn't seem to be a valid option formount
@yanick leaving the structural / strategy discussion aside, what do you think about the PR as is? I think the removal of the |
Going to incorporate this into #375 to avoid an intermediate release |
This PR removes usage of the
svelte/legacy
by implementingrerender
in Svelte 5 via the$state
rune, as recommended in the migration docs.It also adds a "runes mode" component fixture and parameterizes the
render
andrerender
tests to test both "legacy" and "runes mode" components when running in Svelte 5