Skip to content

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .eslintrc.cjs
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ module.exports = {
},
rules: {
'no-undef-init': 'off',
'prefer-const': 'off',
},
},
{
Expand All @@ -49,5 +50,6 @@ module.exports = {
ecmaVersion: 2022,
sourceType: 'module',
},
globals: { $state: 'readonly', $props: 'readonly' },
ignorePatterns: ['!/.*'],
}
2 changes: 0 additions & 2 deletions src/__tests__/fixtures/Comp.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -13,5 +13,3 @@
<h1 data-testid="test">Hello {name}!</h1>

<button on:click={handleClick}>{buttonText}</button>

<style></style>
13 changes: 13 additions & 0 deletions src/__tests__/fixtures/CompRunes.svelte
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>
12 changes: 8 additions & 4 deletions src/__tests__/render.test.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import { render } from '@testing-library/svelte'
import { describe, expect, test } from 'vitest'
import { beforeAll, describe, expect, test } from 'vitest'

import Comp from './fixtures/Comp.svelte'
import { IS_SVELTE_5 } from './utils.js'
import { COMPONENT_FIXTURES, IS_SVELTE_5 } from './utils.js'

describe('render', () => {
describe.each(COMPONENT_FIXTURES)('render $name', ({ component }) => {
const props = { name: 'World' }
let Comp

beforeAll(async () => {
Comp = await import(component)
})

test('renders component into the document', () => {
const { getByText } = render(Comp, { props })
Expand Down
26 changes: 15 additions & 11 deletions src/__tests__/rerender.test.js
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
import { act, render, screen } from '@testing-library/svelte'
import { VERSION as SVELTE_VERSION } from 'svelte/compiler'
import { describe, expect, test, vi } from 'vitest'
import { beforeAll, describe, expect, test, vi } from 'vitest'

import Comp from './fixtures/Comp.svelte'
import { COMPONENT_FIXTURES, IS_SVELTE_5, TYPE_RUNES } from './utils.js'

describe.each(COMPONENT_FIXTURES)('rerender $type', ({ type, component }) => {
let Comp

beforeAll(async () => {
Comp = await import(component)
})

describe('rerender', () => {
test('updates props', async () => {
const { rerender } = render(Comp, { name: 'World' })
const element = screen.getByText('Hello World!')
Expand All @@ -29,13 +34,12 @@ describe('rerender', () => {
)
})

test('change props with accessors', async () => {
const { component, getByText } = render(
Comp,
SVELTE_VERSION < '5'
? { accessors: true, props: { name: 'World' } }
: { name: 'World' }
)
test.skipIf(type === TYPE_RUNES)('change props with accessors', async () => {
const componentOptions = IS_SVELTE_5
? { name: 'World' }
: { accessors: true, props: { name: 'World' } }

const { component, getByText } = render(Comp, componentOptions)
const element = getByText('Hello World!')

expect(element).toBeInTheDocument()
Expand Down
11 changes: 11 additions & 0 deletions src/__tests__/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,14 @@ export const IS_JSDOM = window.navigator.userAgent.includes('jsdom')
export const IS_HAPPYDOM = !IS_JSDOM // right now it's happy or js

export const IS_SVELTE_5 = SVELTE_VERSION >= '5'

export const TYPE_LEGACY = 'legacy'

export const TYPE_RUNES = 'runes'

export const COMPONENT_FIXTURES = [
{ type: TYPE_LEGACY, component: './fixtures/Comp.svelte' },
IS_SVELTE_5
? { type: TYPE_RUNES, component: './fixtures/CompRunes.svelte' }
: [],
].flat()
7 changes: 6 additions & 1 deletion src/pure.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,8 @@ export class SvelteTestingLibrary {
)
props = props.props
}
component.$set(props)

this.rerenderComponent(component, props)
await Svelte.tick()
},
unmount: () => {
Expand All @@ -107,6 +108,10 @@ export class SvelteTestingLibrary {
return component
}

rerenderComponent(component, nextProps) {
component.$set(nextProps)
}

cleanupComponent(component) {
const inCache = this.componentCache.delete(component)

Expand Down
4 changes: 2 additions & 2 deletions src/svelte5-index.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/* eslint-disable import/export */
import { act } from './pure.js'
import { cleanup } from './svelte5.js'
import { cleanup } from './svelte5.svelte.js'

// If we're running in a test runner that supports afterEach
// then we'll automatically run cleanup afterEach test
Expand All @@ -20,4 +20,4 @@ export * from '@testing-library/dom'
// export svelte-specific functions and custom `fireEvent`
// `fireEvent` must be a named export to take priority over wildcard export above
export { act, fireEvent } from './pure.js'
export { cleanup, render } from './svelte5.js'
export { cleanup, render } from './svelte5.svelte.js'
30 changes: 0 additions & 30 deletions src/svelte5.js
Copy link
Collaborator Author

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

This file was deleted.

49 changes: 49 additions & 0 deletions src/svelte5.svelte.js
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
Copy link
Collaborator Author

@mcous mcous May 13, 2024

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 for mount


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
Copy link
Collaborator Author

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?

Copy link
Collaborator

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

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

Copy link
Collaborator Author

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 binds 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

Copy link
Collaborator Author

@mcous mcous May 13, 2024

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:

#375

Copy link
Collaborator

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. :-)

Copy link
Collaborator Author

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

Loading