-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix!: align target
and baseElement
options with testing-library
#325
Conversation
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Can you also prepare a branch for the documentation changes related to this PR?
10-4, will undraft this PR when corresponding docs PR is up |
Docs PR up at testing-library/testing-library-docs#1369 |
Giving this one last smoke test in a real-life suite, will update here with results |
Passed our ~1000 test suite without any unexpected failures (still on Svelte v4). Had exactly one failure: a test that was using Switching the the new |
component.$$.on_destroy.push(() => { | ||
this.componentCache.delete(component) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I just remove this now? It's a weird little bit of logic that makes it so one can call either component.$destroy
or unmount
in a test. It feels reasonable to only provide one way - unmount
- to do this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd tend to say "nah". It's been the established behavior, and with Svelte 4 probably being eclipsed by Svelte 5 soon, there is little gain to change the behavior now.
@yanick heads up, looks like semantic release missed this:
Suspected causes:
Not sure the best way to fix this up, the quickest / easiest might be a little distasteful history rewrite on |
🎉 This PR is included in version 4.2.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes #312, fixes #313
BREAKING CHANGES: The
container
option has been renamed tobaseElement
,result.container
is now set totarget
rather thanbaseElement
,and
render
will now throw if you mix props with thetarget
option.Change log
renderOptions.container
torenderOptions.baseElement
baseElement
asresult.baseElement
baseElement
tocomponentOptions.target
if used, elsedocument.body
componentOptions.target
asresult.container
, which is one layer lower in the DOM than the previous behavior and matches othertesting-library
frameworkscomponentOptions.target
will now trigger an error if it is mixed with props, matching the behavior of other component options