-
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,5 +13,3 @@ | |
<h1 data-testid="test">Hello {name}!</h1> | ||
|
||
<button on:click={handleClick}>{buttonText}</button> | ||
|
||
<style></style> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
<script> | ||
let { name = 'World' } = $props() | ||
|
||
let buttonText = $state('Button') | ||
|
||
function handleClick() { | ||
buttonText = 'Button Clicked' | ||
} | ||
</script> | ||
|
||
<h1 data-testid="test">Hello {name}!</h1> | ||
|
||
<button onclick={handleClick}>{buttonText}</button> |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
/** eslint-global: $state */ | ||
import { mount, unmount } from 'svelte' | ||
|
||
import { SvelteTestingLibrary } from './pure.js' | ||
|
||
class Svelte5TestingLibrary extends SvelteTestingLibrary { | ||
svelteComponentOptions = [ | ||
'target', | ||
'props', | ||
'events', | ||
'context', | ||
'intro', | ||
'recover', | ||
] | ||
Comment on lines
+7
to
+14
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||
|
||
propsByComponent = new Map() | ||
|
||
renderComponent(ComponentConstructor, componentOptions) { | ||
const props = $state(componentOptions.props ?? {}) | ||
const component = mount(ComponentConstructor, { | ||
...componentOptions, | ||
props, | ||
}) | ||
|
||
this.componentCache.add(component) | ||
this.propsByComponent.set(component, props) | ||
|
||
return component | ||
} | ||
|
||
rerenderComponent(component, nextProps) { | ||
const prevProps = this.propsByComponent.get(component) | ||
Object.assign(prevProps, nextProps) | ||
} | ||
|
||
cleanupComponent(component) { | ||
const inCache = this.componentCache.delete(component) | ||
this.propsByComponent.delete(component) | ||
|
||
if (inCache) { | ||
unmount(component) | ||
} | ||
} | ||
} | ||
|
||
const instance = new Svelte5TestingLibrary() | ||
|
||
export const render = instance.render.bind(instance) | ||
export const cleanup = instance.cleanup.bind(instance) | ||
Comment on lines
+46
to
+49
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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)
}
}
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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
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 commentThe reason will be displayed to describe this comment to others. Learn more.
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 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.
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 commentThe reason will be displayed to describe this comment to others. Learn more. Threw up a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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 commentThe 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