-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat: add asFragment
return value from render
#192
Conversation
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.
This is great 💯 Just a quick question.
src/index.js
Outdated
@@ -29,6 +30,7 @@ function render(ui, {container, baseElement = container, queries} = {}) { | |||
// Intentionally do not return anything to avoid unnecessarily complicating the API. | |||
// folks can use all the same utilities we return in the first place that are bound to the container | |||
}, | |||
asFragment: () => JSDOM.fragment(container.innerHTML), |
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.
Is there a reason to do this with JSDOM rather than:
const fragment = document.createDocumentFragment()
fragment.innerHTML = container.innerHTML
That way we don't have to rely on JSDOM directly. I want this package to be capable of running in the browser.
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.
Great point! Pushed a new commit using plain DOM APIs for it.
Followed https://stackoverflow.com/a/25214113/1850276, but JSDOM doesn't support Ranges (jsdom/jsdom#317), so I added a fallback for it.
(I enjoy the fact that it was JSDOM's maintainer who asked the question on SO)
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.
Cool! Thanks! :)
Would you like to add yourself to the contributors table? |
🎉 This PR is included in version 5.2.0 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
@@ -29,6 +29,17 @@ function render(ui, {container, baseElement = container, queries} = {}) { | |||
// Intentionally do not return anything to avoid unnecessarily complicating the API. | |||
// folks can use all the same utilities we return in the first place that are bound to the container | |||
}, | |||
asFragment: () => { | |||
if (typeof document.createRange === 'function') { |
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.
might wanna add a /* istanbul ignore if */
here, and leave a comment pointing back to jsdom/jsdom#317
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.
Thanks! I've just committed that 👍
What:
Adds the possibility to get a
DocumentFragment
version of the rendered component.Why:
I have a test of a connected redux component, and I want to snapshot the diff between them after dispatching an action. Since
container
is a live binding, itsinnerHTML
changes, meaning comparison becomes meaningless.An extra advantage is IMO that you don't need to do
firstChild
on snapshots, as having theFragment
wrapper makes sense. At least in my mind 🙂How:
By upgrading
kcd-scripts
to a version with Jest 23, and adding a dependency onJSDOM
which matches the one found in Jest (to not rely on dependency hoisting).Checklist: