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

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

Jump to bottom

Merged

yanick merged 1 commit into testing-library:next from mcous:align-render-options

Mar 17, 2024

Merged

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

yanick merged 1 commit into testing-library:next from mcous:align-render-options

Mar 17, 2024

Conversation

@mcous

Copy link

Collaborator

@mcous mcous commented Feb 15, 2024 •

edited

Loading

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

mcous commented

Feb 15, 2024

View reviewed changes

package.json Show resolved Hide resolved

src/__tests__/__snapshots__/render.test.js.snap Show resolved Hide resolved

src/__tests__/fixtures/Comp.svelte Show resolved Hide resolved

src/__tests__/fixtures/Comp2.svelte Show resolved Hide resolved

src/__tests__/fixtures/Rerender.svelte Show resolved Hide resolved

src/__tests__/render.test.js Outdated Show resolved Hide resolved

src/pure.js Outdated Show resolved Hide resolved

src/__tests__/rerender.test.js Show resolved Hide resolved

@mcous

Copy link

Collaborator Author

mcous commented Feb 16, 2024

@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

yanick

yanick approved these changes

Feb 16, 2024

View reviewed changes

Copy link

Collaborator

@yanick yanick left a 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?

@mcous

Copy link

Collaborator Author

mcous commented Feb 16, 2024

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

@mcous mcous mentioned this pull request

Feb 17, 2024

docs(svelte-testing-library): update API docs for v5 release testing-library/testing-library-docs#1369

Merged

@mcous mcous marked this pull request as ready for review

February 17, 2024 18:58

@mcous

Copy link

Collaborator Author

mcous commented Feb 17, 2024

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

@mcous

Copy link

Collaborator Author

mcous commented Feb 17, 2024

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

@mcous mcous marked this pull request as draft

February 17, 2024 19:04

@mcous

Copy link

Collaborator Author

mcous commented Feb 17, 2024 •

edited

Loading

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

@mcous mcous marked this pull request as ready for review

February 17, 2024 19:29

@mcous mcous mentioned this pull request

Feb 20, 2024

fix: $destroy and createRoot are no more #328

Merged

@yanick yanick mentioned this pull request

Feb 28, 2024

merge branch-align-render-options with next #331

Closed

@yanick

Copy link

Collaborator

yanick commented Feb 28, 2024

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

@mcous


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

b257b1f

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.

@mcous

Copy link

Collaborator Author

mcous commented Mar 16, 2024

@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 mcous requested a review from yanick

March 16, 2024 16:02

mcous

mcous commented

Mar 16, 2024

View reviewed changes

src/pure.js

Comment on lines +103 to +105

component.$$.on_destroy.push(() => {

this.componentCache.delete(component)

})

Copy link

Collaborator Author

@mcous mcous Mar 16, 2024

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

Copy link

Collaborator

@yanick yanick Mar 17, 2024

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.

@mcous mcous mentioned this pull request

Mar 16, 2024

test: remove unnecessary tests and skips #338

Merged

@yanick yanick merged commit 8d98aa5 into testing-library:next

Mar 17, 2024

@mcous

Copy link

Collaborator Author

mcous commented Mar 17, 2024

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

[2:54:51 PM] [semantic-release] [@semantic-release/commit-analyzer] › i 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] › i 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