fix!: Rerender to only update props of component without destroying it

[basarbk]

When rerendering the component, it shouldn't be destroyed and re mounted. Browser behavior is not working like that, and current stl implementation can lead false positive test results. Possible fail case is reproduced in this repo
In that scenario UserPage is visible for paths /user/:id . Corresponding routing setup would be like this (either with svelte-routing or svelte-navigator). We can pass the id as param

<Route path="/user/:id" let:params>
   <UserPage id={params.id} />
</Route>

When we test this on browser and we navigate from /user/1 to /user/2, UserPage component is not destroyed. Only the prop, id is changed and same component is used. And If we want to load corresponding user data from backend, we must be making another api call with this new id.

But in test, since rerender is destroying and mounting the component, even an incorrect implementation like this one is working. So it ends up with false positive result.

With this change, since rerender can be called only for a rendered component, when it is called, just updating the component's props.
I'm not sure how many other use cases are there other than updating the props, but with the fix, just updating the props of component.
And since this prop change is async, calling tick as well which makes rerender to wait for tick.

If this impl approved then we need to update the rerender part here in this doc

[yanick]

I'm in favor of merging this in, with three notes/caveats:

• we should add some more tests. (perhaps steal from https://github.com/basarbk/stl-rerender-issue?)
• we should label this change a breaking change
• we should update the documentation as mentioned above

[mcous]

@yanick how are you feeling about the breaking change? We could implement this with a {preserve: true} option and keep it as a minor bump to reduce churn, if that felt useful

[yanick]

@yanick how are you feeling about the breaking change?

I'm leaning toward doing it. It's kinda too bad that it'll make our major version out of sync with Svelte's, but that's something that will happen sooner or later anyway.

We could implement this with a {preserve: true} option and keep it as a minor bump to reduce churn, if that felt useful

In this case I don't think it's necessary as I don't see that many people actually realized the components were destroyed on re-rendering and actually wanted that.

Sounds sane?

[yanick]

Documentation branch: https://github.com/yanick/testing-library-docs/tree/rerender

[mcous]

I'm leaning toward doing it. [...] Sound sane?

@yanick I'm 100% in agreement, especially after working on #314. The current rerender is a bit useless and can be easily replaced by unmount + render for those that want that behavior. It looks like the current implementation started as an attempt to replicate (P)react, which as a VDOM lib is just fundamentally different

I'm not in a huge personal hurry here, given there's an acceptable workaround in await act(() => component.$set(props)). But it also seems like semantic release is already configured with support for version branches and a next branch, so maybe it's time to cut that branch and start collecting changes

[mcous]

I don't think we need to accept all options here, I think we should just accept props.

This would match the signature of vue-testing-library's rerender method, so I think the sibling consistency would be worthwhile

[yanick]

I agree only passing props makes sense. However, this is a change that will break the code of anybody using rerender, so I'm adding a small check that will see if the user passed { props: { ... } } and warn them that's deprecated. And we can hard phase out that signature on the next major update.

[mcous]

Followup on my other suggestion; I think this would become

Suggested change

	rerender: (options: SvelteComponentOptions<C>) => Promise<void>
	rerender: (props: ComponentProps<C>) => Promise<void>

[yanick]

But it also seems like semantic release is already configured with support for version branches and a next branch, so maybe it's time to cut that branch and start collecting changes

I wouldn't be above releasing it by itself, but yeah, let's use it as a guinea pig for the next branch.

[github-actions]

🎉 This PR is included in version 4.2.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀