feat(render): deprecate renderIntoDocument and make it the default #118

kentcdodds · 2018-06-18T23:26:43Z

What: deprecate renderIntoDocument and make it the default

Why: Closes #116

How:

Code refactor
My son is waiting for me... Otherwise I'd be more informative.

Checklist:

Documentation
Tests
Ready to be merged
Added myself to contributors table N/A

cc @hnordt @sompylasar

codecov · 2018-06-18T23:29:06Z

Codecov Report

Merging #118 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #118   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          17     18    +1     
  Branches        0      2    +2     
=====================================
+ Hits           17     18    +1

Impacted Files	Coverage Δ
src/index.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f1ce48...777cfd3. Read the comment docs.

hnordt · 2018-06-18T23:52:19Z

The README needs to be updated. I can do it if you want.

kentcdodds · 2018-06-19T00:00:48Z

What part? This PR makes major updates to the README.

hnordt · 2018-06-19T00:04:21Z

@kentcdodds nevermind, it's there, I didn't see.

sompylasar · 2018-06-19T00:23:57Z

README.md

+option, it will not be appended to the `document.body` automatically.
+
+**baseElement**: This is used as the base element for the queries as well as
+what is printed when you use `debug()`.


Which one is the default for this baseElement?

sompylasar · 2018-06-19T00:25:30Z

README.md

-
-renderIntoDocument(<div />)
-```
+> `getByText` don't work for your use case. Using data-testid attributes do not


Add backticks around data-testid on this line, too?

sompylasar · 2018-06-19T00:26:50Z

README.md

-
-</details>
+Failing to call `cleanup` when you've called `render` could result in a memory
+leak and tests which are not `idempotent` (which can lead to difficult to debug


"idempotent" is not a code identifier, use regular quotes instead of backticks here?

sompylasar · 2018-06-19T00:27:54Z

README.md

-> to attach event handlers to the rendered node rather than the `document`.
-> Learn more here:
+> NOTE: If you don't like having to use `cleanup` (which we have to do because
+> we render into document.body) to get `fireEvent` working, then feel free to


add backticks around document.body

sompylasar · 2018-06-19T00:32:05Z

README.md

+
+  // wait until the callback does not throw an error. In this case, that means
+  // it'll wait until we can get a form control with a label that matches "username"
+  await wait(() => expect(queryByText(/loading.../i).not.toBeInTheDOM())


it'll wait until we can get a form control with a label that matches "username" does not sound aligned with the code it comments: it actually waits for "loading..." to disappear from the DOM, and only after that we look for the form control on the next line

/loading.../i – the ... matches three arbitrary characters instead of the expected three ASCII dots, worth escaping?

sompylasar · 2018-06-19T00:35:09Z

src/__tests__/forms.js

  // then ensure that there's a submit button.
-  Simulate.submit(formNode)
+  fireEvent.click(submitButtonNode)


According to the NOTE comment above, clicking on the submit button won't work in jsdom. Does fireEvent.click take care of that?

sompylasar · 2018-06-19T00:39:18Z

src/index.js

+// maybe one day we'll expose this (perhaps even as a utility returned by render).
+// but let's wait until someone asks for it.
+function cleanupAtContainer(container) {
+  document.body.removeChild(container)


If container is custom passed by the caller, thus not appended to the document.body by render, the caller may not expect that it will be removed by cleanup which should be a mirror to render.

Hmmm... So what are you suggesting?

Oh, I see what you mean. I'm not sure how to solve that issue reliably. If they don't like the behavior then they can file an issue and we can add an option to prevent this. Let's wait to add complexity until someone has an issue.

Usually if a piece of code owns the lifecycle of an object (the caller in the case of custom container; the module with the render function in the case of default container), that piece is responsible for both creation+setup and teardown+destruction.

The suggestion is to make sure not to remove the container from the DOM if it hasn't been added to the DOM by the render module, only remove the map entry (lifecycle of which is owned by the render module) and ReactDOM.unmountComponentAtNode (which is a mirror to ReactDOM.render, mounted component's lifecycle is also owned by the render module).

I understand that the object/memory ownership and the allocate/deallocate pattern is often missed/violated in the JavaScript community due to various reasons, so it may look odd to require following it from this library users.

That makes sense. Perhaps we should not add items to the set if we don't append it to the body. Would you like to open a PR for that?

Hmmmm..... I'm still not certain this is the behavior people would expect our want 🤔

Okay then, let's assume people move the ownership of the container from their code to the library code by calling render with that container.

What if instead we did:

container.parentElement && container.parentElement.removeChild(container)

I think that would make sense to happen in the cleanup regardless of whether it's in the body or not.

That would make sense to shorten the caller code to let it manage only container creation, and, my point still holds, the ownership of the container object lifecycle is moved from the caller into the render function.

I'm a little confused by what you're suggesting. But why don't you make a PR and maybe that'll clear things up for me.

sompylasar · 2018-06-19T00:43:38Z

typings/index.d.ts

@@ -46,15 +46,14 @@ export interface RenderResult extends GetsAndQueries {
  debug: () => void
  rerender: (ui: React.ReactElement<any>) => void
  unmount: VoidFunction
+  cleanup: VoidFunction


Is this an established practice to have a single type definition for functions with different purposes? I would avoid sharing the type because these functions, although look similar, are not the same thing. For example, debug looks like a VoidFunction but the type is not shared. Oh by the way, I don't see a definition of VoidFunction in this file, where is it coming from?

This was a mistake on my part. Originally I had added a helper cleanup utility, then thought better of it because nobody's asking for that additional helper and not enough people would use it to be worth the docs.

Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).

kentcdodds · 2018-06-19T04:19:55Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

sompylasar · 2018-06-19T05:37:46Z

I saw you removed the cleanup: VoidFunction, but the unmount: VoidFunction is still there in the index.d.ts TypeScript definition file, without a visible definition of the VoidFunction type – where does this type come from?

kentcdodds · 2018-06-19T05:53:51Z

where does this type come from?

No idea 🤷‍♂️

jomaxx · 2018-06-19T15:33:25Z

@kentcdodds this is amazing! happy to see renderIntoDocument become the default render

antsmartian · 2018-06-19T15:45:29Z

Wow this is great @kentcdodds ! 👍

* waitForDomChange is implemented along with tests, documentation, and types. * Implements changes requested on 10-9-18. Removes matchSnapshots. Correctly formats README.md. Changes Promise to return a mutationsList. Adds documentation for mutationsList. * causes waitForElement to throw an error if callback is not defined. changes tests to account for new behavior. removes the documentation around the default callback. adds MutationObserver documentation for waitForDomChange

kentcdodds requested review from johann-sonntagbauer, antsmartian, pbomb and jomaxx June 18, 2018 23:26

sompylasar reviewed Jun 19, 2018

View reviewed changes

feat(render): deprecate renderIntoDocument and make it the default

777cfd3

Closes #116 BREAKING CHANGE: `renderIntoDocument` replaces `render` and `Simulate` is no longer exported (use `fireEvent` instead).

kentcdodds force-pushed the pr/render-only branch from bb61e94 to 777cfd3 Compare June 19, 2018 03:59

kentcdodds merged commit 5257500 into master Jun 19, 2018

kentcdodds deleted the pr/render-only branch June 19, 2018 05:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(render): deprecate renderIntoDocument and make it the default #118

feat(render): deprecate renderIntoDocument and make it the default #118

kentcdodds commented Jun 18, 2018

codecov bot commented Jun 18, 2018 •

edited

hnordt commented Jun 18, 2018

kentcdodds commented Jun 19, 2018

hnordt commented Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

sompylasar Jun 19, 2018

kentcdodds Jun 19, 2018

kentcdodds Jun 19, 2018

sompylasar Jun 19, 2018

kentcdodds Jun 19, 2018

kentcdodds Jun 19, 2018

sompylasar Jun 19, 2018

kentcdodds Jun 19, 2018

sompylasar Jun 19, 2018

kentcdodds Jun 19, 2018

sompylasar Jun 19, 2018

kentcdodds Jun 19, 2018

kentcdodds commented Jun 19, 2018

sompylasar commented Jun 19, 2018

kentcdodds commented Jun 19, 2018

jomaxx commented Jun 19, 2018

antsmartian commented Jun 19, 2018

feat(render): deprecate renderIntoDocument and make it the default #118

feat(render): deprecate renderIntoDocument and make it the default #118

Conversation

kentcdodds commented Jun 18, 2018

codecov bot commented Jun 18, 2018 • edited

Codecov Report

hnordt commented Jun 18, 2018

kentcdodds commented Jun 19, 2018

hnordt commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Jun 19, 2018

sompylasar commented Jun 19, 2018

kentcdodds commented Jun 19, 2018

jomaxx commented Jun 19, 2018

antsmartian commented Jun 19, 2018

codecov bot commented Jun 18, 2018 •

edited