fix!: align `target` and `baseElement` options with testing-library

testing-library / svelte-testing-library

:chipmunk: Simple and complete Svelte DOM testing utilities that encourage good testing practices

https://testing-library.com/docs/svelte-testing-library/intro

MIT License

615 stars 33 forks source link

fix!: align `target` and `baseElement` options with testing-library #325

Closed mcous closed 6 months ago

mcous commented 7 months ago

Fixes #312, fixes #313

BREAKING CHANGES: The container option has been renamed to baseElement, result.container is now set to target rather than baseElement, and render will now throw if you mix props with the target option.

Change log

Renamed renderOptions.container to renderOptions.baseElement
Return baseElement as result.baseElement
Default baseElement to componentOptions.target if used, else document.body
Return componentOptions.target as result.container, which is one layer lower in the DOM than the previous behavior and matches other testing-library frameworks
componentOptions.target will now trigger an error if it is mixed with props, matching the behavior of other component options
Updated and reorganized tests
Updated type definitions

mcous commented 7 months ago

@yanick let me know if you think this is too much to change in one PR; I can split it up into a few separate changes relatively trivially

mcous commented 7 months ago

10-4, will undraft this PR when corresponding docs PR is up

mcous commented 7 months ago

Docs PR up at testing-library/testing-library-docs#1369

mcous commented 7 months ago

Giving this one last smoke test in a real-life suite, will update here with results

mcous commented 7 months ago

Passed our ~1000 test suite without any unexpected failures (still on Svelte v4). Had exactly one failure: a test that was using rerender and relying on the old synchronous destroy and remount behavior. Fixing this test, in turn, uncovered an issue with the type definitions for rerender - in reality, it accepts Partial<Props>, not Props.

Switching the the new await rerender(...) fixed the test, and I just pushed the type definitions fix, so I think we're all good here if CI agrees with me

yanick commented 7 months ago

@mcous I tried to merge your branch with the head of next on #331 I think I got it all?

mcous commented 6 months ago

@yanick thanks for the assist. I squashed and rebased this branch while referencing #331, which was a little easier on my brain. It looked like 331 added has a few duplicate test cases, but otherwise I mostly followed the changes there

mcous commented 6 months ago

@yanick heads up, looks like semantic release missed this:

[2:54:51 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  Analyzing commit: fix!: align `target` and `baseElement` options with testing-library (#325)

Fixes #312, fixes #313

BREAKING CHANGES: The `container` option has been renamed to `baseElement`,
  `result.container` is now set to `target` rather than `baseElement`,
  and `render` will now throw if you mix props with the `target` option.
[2:54:51 PM] [semantic-release] [@semantic-release/commit-analyzer] › ℹ  The commit should not trigger a release

Suspected causes:

semantic release does not recognize the ! as a breaking change in fix!: ..., so it didn't see fix nor that it was a major bump
I accidentally wrote BREAKING CHANGES: instead of BREAKING CHANGE: in the footer, so it also missed the major bump here

Not sure the best way to fix this up, the quickest / easiest might be a little distasteful history rewrite on next

github-actions[bot] commented 6 months ago

:tada: This PR is included in version 4.2.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: