Component options check omits `target`
Closed this issue ยท 4 comments
Overview
The render
function of svelte-testing-library
has two "modes":
- Pass props, e.g.
render(Comp, { name: 'hello' })
- Pass component options, e.g.
render(Comp, { intro: true, props: { name: 'hello' } })
To do this, it checks the passed in options object against a list of known Svelte options, and if it detects one, it goes into "options mode". Otherwise, it uses "props mode."
The check list is missing target
, which means tests for a component that has a prop named target
will unexpectedly fail in "props mode".
Expected behavior
target
cannot be intermixed with props, just like any other Svelte option:
// triggers error, must use `{ intro: true, props: { name: 'hello' } }`
render(Comp, { name: 'hello', intro: true })
// should trigger error, must use `{ target: ..., props: { name: 'hello' } }`
render(Comp, { name: 'hello', target: someElement })
Actual behavior
The target
option can be intermixed with props, where "target" will be pulled out and used as the DOM target and never passed to the component.
Proposed change
Any change here should probably be considered a breaking change to the render
API.
The smallest fix I can think of here is to simply add a check for target
.
A slightly larger change would be to switch away from a checklist-based approach to a simple check for the props
key. If props
exists, treat the object as "options mode," otherwise treat is as "props" mode. The downside of this approach is that you'd be forced to specify an empty props object to trigger options mode, which feels annoying enough that it might disqualify the idea.
An even larger change could be to make the second render argument props exclusively, and move all options (both Svelte and render) to the third argument
The check list is missing target
Ah, but then the target
is already removed thanks to
const render = (
Component,
{ target, ...options } = {}, // <=== sneaky removal of target
{ container, queries } = {}
) => {
so target
is never passed to checkProps
. Although target
should probably be added to svelteComponentOptions
for completeness' sake.
I'm wondering.... render
with its DYIM first argument is darn useful, but we might want to provide people with two alternatives, renderStrict
which always takes the options object as its first argument, and renderProps
which always takes props directly as its first argument. So anybody who feels that render
itself is too loosey-goosey can do
import { renderStrict as render } from '@testing-library/svelte';
@yanick I think the sneaky removal of target
is the problem! As a user, you expect to be able to do either:
// 1. props as options
render(Comp, { someProp: 'foo' })
// 2. explicit props
render(Comp, { props: { someProp: 'foo' } })
If you want to name a prop the same as a component option, you need to use the second form. However, because target
is pulled off early, the first form can silently fail - or fails in a weird manner - rather than explicitly failing due to the options check.
If you write a component that has a prop named target
:
<script>
export let someProp
export let target
</script>
<!-- ... -->
Then you may write the following call to render
:
// I think this will work, and `render` will not yell at me directly,
// but unbeknownst to me, `target` is pulled off and
// used in a different manner than I expect
render(Comp, { target: ..., someProp: ... })
After sitting with this for a bit, I think we only need a small fix: target
should be checked along with all the other options and then pulled off
After sitting with this for a bit, I think we only need a small fix:
target
should be checked along with all the other options and then pulled off
Aaaah, I see. And yup, I agree, that fix should do the trick.
๐ This issue has been resolved in version 4.2.2 ๐
The release is available on:
Your semantic-release bot ๐ฆ๐