The returned `container` is one level too high compared to other frameworks

mcous commented 5 months ago

Overview

The render function provides two options for changing how a component gets rendered into the DOM: componentOptions.target and renderOptions.container. It returns a result.container element.

const { container } = render(Component, componentOptions, renderOptions)

componentOptions.target is the target element the component is rendered into
- Defaults to an empty <div> that we create
renderOptions.container is the element that target is inserted into
- Defaults to document.body
result.container returns renderOptions.container

These option names and their behaviors are different in confusing ways from other testing-library frameworks that make svelte-testing-library a little more difficult to use than its siblings. The primary issue is that by default we return document.body rather than the created div for result.container.

Expectations

Value	Svelte	React / Preact / Vue
Document root option	`container`	`baseElement`
Mount point option	`target`	`container`
Returned container	Document root	Mount point

const { container, component } = render(Comp, { foo: 'bar' })

<body>                 <!-- container (baseElement) -->
  <div>                <!-- target (container) -->
    <Comp foo="bar" /> <!-- component -->
  </div>
</body>

The main issue I have is result.container is one level higher than I expect. The most common way I notice this is that I expect container.firstChild to be my component's first node. Instead, it is container.firstElement.firstElement.

This expectation comes from the react-testing-library docs:

container

The containing DOM node of your rendered React Element (rendered using ReactDOM.render). It's a div. This is a regular DOM node, so you can call container.querySelector etc. to inspect the children.

Tip: To get the root element of your rendered element, use container.firstChild.

Proposed changes

Since there are definitely breaking changes on the horizon due to Svelte moving on from v3, I think there's an opportunity here to align the render API with all the other libraries.

I think a small but effective change set would be:

Rename the existing container option to baseElement
Return baseElement (root) as result.baseElement
Return target (the wrapping <div>) as result.container
If target is provided, use it as the baseElement and do not create/insert any other elements
- This is the same behavior as (react|preact|vue)-testing-library

I think there are a few other changes we could consider:

Rename the existing target option to container
- Pro: this would align better with the name of the return value
- Con: would misalign us with the name of the Svelte option
Collapse componentOptions and renderOptions into a single options object
- Pro: aligns a little better with vue-testing-library
- Con: runs the risk of conflicts between Svelte options and testing-library options

Relates to #312

yanick commented 4 months ago

First, kudos for the detailed explanations. Even with that, I had to re-read it many times before my head stopped swimming with the overloaded meanings of target and container. :-)

I think a small but effective change set would be:

Rename the existing container option to baseElement Return baseElement (root) as result.baseElement Return target (the wrapping
) as result.container If target is provided, use it as the baseElement and do not create/insert any other elements This is the same behavior as (react|preact|vue)-testing-library

That all make sense to me. It aligns the terminologies as best as possible.

I think there are a few other changes we could consider:

Rename the existing target option to container Pro: this would align better with the name of the return value Con: would misalign us with the name of the Svelte option

I think we're better to stick with target. In my opinion, Svelte terminology compatibility is the number 1 priority, and alignment with other testing-libraries being number 2. Moreover that with this render right now we can say that the first argument is what we throw verbatim (ish) at the Svelte Component class. If we begin to add exceptions, it'll muddy more the waters than it'll help things (or so I think).

Collapse componentOptions and renderOptions into a single options object Pro: aligns a little better with vue-testing-library Con: runs the risk of conflicts between Svelte options and testing-library options

Yeeeah, it's one of those that sounds good because it's slightly less typing, but what was "first argument is what svelte needs -- you know Svelte and will feel at home there -- second argument is what the testing lib need -- don't worry much, you'll hardly ever use it", will become "there is one argument that is what you're used to, but not quite. Also if Svelte or the testing library change something it'll be a bucket of fun". I approve of the sentiment, but think this path leads to dragons.

github-actions[bot] commented 3 months ago

:tada: This issue has been resolved in version 4.2.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

testing-library / svelte-testing-library