Component options check omits `target`

[mcous]

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

[yanick]

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';

[mcous]

@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

[yanick]

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.

[github-actions]

🎉 This issue has been resolved in version 4.2.2 🎉

The release is available on:

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

Your semantic-release bot 📦🚀