Unmount React component when disconnected. Closes #1260 #1432

NMinhNguyen · 2018-04-17T09:47:11Z

Closes #1260.

Bug
Feature

Requirements

Read the
contribution guidelines.
Wrote tests.
Updated docs and upgrade instructions, if necessary.
Updated TS definitions, if necessary.

Rationale

Why is this PR necessary?

Implementation

Why have you implemented it this way? Did you try any other methods?

I based the implementation on skatejs/renderer-preact#5.
For the test, I looked at how React tests unmountComponentAtNode and how they spy on lifecycle hooks.

Open questions

Are there any open questions about this implementation that need answers?

Is it safe to store renderRoot on this._renderRoot? I didn't want to rely on this.renderRoot being provided via withRenderer, but perhaps it's fine to require withReact to always be used with withRenderer? It does mean that I'd either have to mock it in tests, or couple it to withRenderer though.
~~inside disconnectedCallback, should super.disconnectedCallback() be called first (the way it is in Make sure we notify preact in disconnectedCallback renderer-preact#5) or last?~~ I've done some googling and seems like the super call is always first.
is there any reason why the current renderer-preact in the monorepo does not unmount, unlike the old repo?

treshugart · 2018-04-20T06:20:58Z

I just commented on your issue and didn't realise there was a PR already! Fantastic!

treshugart · 2018-04-20T06:25:17Z

@NMinhNguyen

Is it safe to store renderRoot on this._renderRoot? I didn't want to rely on this.renderRoot being provided via withRenderer, but perhaps it's fine to require withReact to always be used with withRenderer? It does mean that I'd either have to mock it in tests, or couple it to withRenderer though.

Happy for you to rely on renderRoot.

I've done some googling and seems like the super call is always first.

👍

is there any reason why the current renderer-preact in the monorepo does not unmount, unlike the old repo?

Hmm, I don't remember and can't find anything on a quick search of issues. Would you mind reimplementing this? It should just be a copy pasta, which is okay for now.

I'll leave this PR as is and await your response to discuss how we should proceed.

NMinhNguyen · 2018-04-20T09:25:20Z

Thanks for your review!

Happy for you to rely on renderRoot.

What's the best way to expose that in the test though? Should I use it with withRenderer? Or give it a mock class that has a dummy get renderRoot()? Or perhaps even set it on the instance in the class?

// in the test file

const el = new ReactComponentWrapper();
el.renderRoot = el; // expose like this?

It should just be a copy pasta

I guess you'd want me to also use renderRoot there too? But sure, no problem :)

The reason I asked about disconnectedCallback is because I was viewing it as a "destructor" of sorts, so thought it might be best to run it in reverse order to the constructor (super first), but oh well, will leave as is!

NMinhNguyen · 2018-04-21T16:36:07Z

Right, so I cherry-picked the original change from the Preact renderer (to preserve authorship). I then tried to naively change all occurrences of this._renderRoot to this.renderRoot, but obviously without using withRenderer or withComponent it is undefined. I then tried to wrap everything in withRenderer in the tests, but that meant having to change the first argument to el.renderer(el to be el.renderer(el.renderRoot, as well as changing all el.innerHTML references to be el.renderRoot.innerHTML. I've pushed the commit (9bd696b) anyway, to see what you think, but I'm not sure I like it: mainly because it has this implicit dependency on withRenderer to define renderRoot, but also because it somewhat complicates the tests 😕

NMinhNguyen · 2018-04-21T16:39:12Z

packages/renderer-react/src/index.js

@@ -1,6 +1,5 @@
 import React from 'react';
 import { render, unmountComponentAtNode } from 'react-dom';
-import { withRenderer } from 'skatejs';


This skatejs import didn't seem to be used in this module, so I decided to remove it, along with the dependency on skatejs in package.json. But since withRenderer from skatejs was now being used in tests, I actually moved skatejs to devDependencies. Please let me know if you think removing this import is unsafe, e.g. if there's some ordering issue that this import was trying to work around, or if there's some side effects when skatejs is imported

Sounds good.

treshugart · 2018-04-22T22:26:09Z

packages/renderer-preact/src/__tests__/index.js


-class MyElement extends withRenderer() {
+class MyElement extends withRenderer(withPreact()) {


I don't mind using withRenderer here in the tests. On the surface it seems to simplify it, but I'm wondering why this didn't work before? It seems if you remove the dependency on withRenderer and the accessing of renderRoot, things would still work, no?

You mean, just use the original code where root was being stored on this._renderRoot? I only updated it for consistency with the requested changes for React renderer really, but I'm happy to revert.

Interestingly, the current tests still pass with the accessing of renderRoot, would you rather I reverted these last changes to the Preact renderer tests? Or perhaps the accessing of renderRoot too?

I've reverted the renderRoot changes to renderer-preact, and left renderer-react as is. Does make we wonder why not have the same implementation for the React renderer though 😛

treshugart · 2018-04-23T02:15:56Z

Hey @NMinhNguyen I've updated the tests in c8b0006 to be as consistent as possible. Hopefully this simplifies the work you have to do. I don't think I originally grokked your intention and probably responded slightly out of context.

NMinhNguyen · 2018-04-23T02:20:31Z

@treshugart no worries. Just to clarify, what changes would you like me to make? Do you still want to use renderRoot that comes from withRenderer in both renderers? Or just store it on an underscore-prefixed instance property like in the Preact renderer, so that it can be referenced when unmounting?

treshugart · 2018-04-23T02:46:07Z

Or just store it on an underscore-prefixed instance property like in the Preact renderer, so that it can be referenced when unmounting?

Right, this is the part I misunderstood, apologies. I thought you meant in the tests. For the unmounting you can just store it in the underscore prefixed like the Preact renderer. Reusing renderRoot isn't possible because, like you said, it would assume the usage of withRenderer().

Thanks again, and sorry for the run-around.

NMinhNguyen · 2018-04-23T02:47:47Z

@treshugart ah I think I might know where your confusion's coming from - it doesn't help that in the renderer tests, each renderer is called withRenderer, rather than withReact or withPreact 😅 Should in theory be as simple as reverting that last commit.

NMinhNguyen · 2018-04-23T02:58:59Z

packages/renderer-preact/package.json

@@ -3,7 +3,8 @@
  "browser": "dist/es/index.js",
  "description": "A SkateJS renderer for Preact.",
  "devDependencies": {
-    "preact": "8.2.7"
+    "preact": "8.2.7",
+    "skatejs": "^5.1.2"


Added this since the tests are now pulling in skatejs in c8b0006#diff-61ecfdc8a7d04b1bf1fbefe49953d7feR4

NMinhNguyen · 2018-04-23T03:00:13Z

packages/renderer-preact/src/__tests__/index.js

+    }
+  }
+
+  @define


made this test look more like c8b0006

NMinhNguyen · 2018-04-23T03:04:03Z

packages/renderer-react/src/index.js

+    disconnectedCallback() {
+      super.disconnectedCallback && super.disconnectedCallback();
+      unmountComponentAtNode(this._renderRoot);
+      this._renderRoot = null;


kinda got this habit from React where refs are nulled out when unmounting - prob a good idea to prevent any memory leaks. Could also add this line to the Preact renderer if you want?

Yeah, that'd be great. I'll merge this to get it in and we can do that as a separate change. Should we test that it's nulled out you think?

Sure, can do. It feels almost like testing an implementation detail (expect(el._renderRoot).toBeNull()), but perhaps that's not so bad

NMinhNguyen · 2018-04-23T03:07:58Z

Ready for another review. Will happily address any further comments.

…g disconnected. See skatejs#1432 (comment)

…nnect. See skatejs#1432 (comment)

developit · 2018-04-23T12:48:10Z

packages/renderer-preact/src/index.js

+    disconnectedCallback() {
+      super.disconnectedCallback && super.disconnectedCallback();
+      // Preact hack https://github.com/developit/preact/issues/53
+      const Nothing = () => null;


I believe Preact will happily accept null as the first argument to render() these days, the component should no longer be needed.

Here's an example: https://jsfiddle.net/developit/xngr7pm1/

See skatejs#1432 (comment)

NMinhNguyen force-pushed the unmount-react-component-when-disconnected branch from 29ef3c0 to 50da9ec Compare April 21, 2018 01:09

NMinhNguyen commented Apr 21, 2018

View reviewed changes

treshugart reviewed Apr 22, 2018

View reviewed changes

NMinhNguyen force-pushed the unmount-react-component-when-disconnected branch from 9bd696b to a632f4e Compare April 23, 2018 00:48

NMinhNguyen force-pushed the unmount-react-component-when-disconnected branch 2 times, most recently from daf7280 to d510ec4 Compare April 23, 2018 02:45

NMinhNguyen and others added 2 commits April 23, 2018 03:54

Unmount React component when disconnected. Closes skatejs#1260

55c8eb4

Clean any Preact components when CE is removed.

2de8a54

NMinhNguyen force-pushed the unmount-react-component-when-disconnected branch from d510ec4 to 2de8a54 Compare April 23, 2018 02:56

NMinhNguyen commented Apr 23, 2018

View reviewed changes

packages/renderer-preact/src/__tests__/index.js

}

}

@define

Copy link

Member Author

NMinhNguyen Apr 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

made this test look more like c8b0006

NMinhNguyen commented Apr 23, 2018

View reviewed changes

treshugart approved these changes Apr 23, 2018

View reviewed changes

treshugart merged commit 4ad3b9b into skatejs:master Apr 23, 2018

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot after bein…

4459a5e

…g disconnected. See skatejs#1432 (comment)

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot post disco…

5ece9fe

…nnect. See skatejs#1432 (comment)

NMinhNguyen mentioned this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot post disco… #1433

Closed

6 tasks

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot post disco…

cb18216

…nnect. See skatejs#1432 (comment)

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot post disco…

305ac0e

…nnect. See skatejs#1432 (comment)

NMinhNguyen mentioned this pull request Apr 23, 2018

Ensure the (p)react renderers do not hold on to renderRoot post disco… #1434

Merged

6 tasks

developit reviewed Apr 23, 2018

View reviewed changes

NMinhNguyen deleted the unmount-react-component-when-disconnected branch April 24, 2018 08:57

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 24, 2018

Simplify unmounting Preact components.

81abff0

See skatejs#1432 (comment)

NMinhNguyen mentioned this pull request Apr 24, 2018

Simplify unmounting Preact components. #1435

Merged

7 tasks

NMinhNguyen added a commit to NMinhNguyen/skatejs that referenced this pull request Apr 24, 2018

Simplify unmounting Preact components.

e1f9a0b

See skatejs#1432 (comment)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unmount React component when disconnected. Closes #1260 #1432

Unmount React component when disconnected. Closes #1260 #1432

NMinhNguyen commented Apr 17, 2018 •

edited

treshugart commented Apr 20, 2018 •

edited

treshugart commented Apr 20, 2018 •

edited

NMinhNguyen commented Apr 20, 2018 •

edited

NMinhNguyen commented Apr 21, 2018 •

edited

NMinhNguyen Apr 21, 2018 •

edited

treshugart Apr 22, 2018

treshugart Apr 22, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited

treshugart commented Apr 23, 2018

NMinhNguyen commented Apr 23, 2018

treshugart commented Apr 23, 2018

NMinhNguyen commented Apr 23, 2018

NMinhNguyen Apr 23, 2018

NMinhNguyen Apr 23, 2018

NMinhNguyen Apr 23, 2018

treshugart Apr 23, 2018

NMinhNguyen Apr 23, 2018

NMinhNguyen commented Apr 23, 2018

developit Apr 23, 2018


		class MyElement extends withRenderer() {
		class MyElement extends withRenderer(withPreact()) {

+                  }
+                }
+                @define

Unmount React component when disconnected. Closes #1260 #1432

Unmount React component when disconnected. Closes #1260 #1432

Conversation

NMinhNguyen commented Apr 17, 2018 • edited

Requirements

Rationale

Implementation

Open questions

treshugart commented Apr 20, 2018 • edited

treshugart commented Apr 20, 2018 • edited

NMinhNguyen commented Apr 20, 2018 • edited

NMinhNguyen commented Apr 21, 2018 • edited

NMinhNguyen Apr 21, 2018 • edited

Choose a reason for hiding this comment

treshugart Apr 22, 2018

Choose a reason for hiding this comment

treshugart Apr 22, 2018 • edited

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018 • edited

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018 • edited

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018 • edited

Choose a reason for hiding this comment

treshugart commented Apr 23, 2018

NMinhNguyen commented Apr 23, 2018

treshugart commented Apr 23, 2018

NMinhNguyen commented Apr 23, 2018

NMinhNguyen Apr 23, 2018

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018

Choose a reason for hiding this comment

treshugart Apr 23, 2018

Choose a reason for hiding this comment

NMinhNguyen Apr 23, 2018

Choose a reason for hiding this comment

NMinhNguyen commented Apr 23, 2018

developit Apr 23, 2018

Choose a reason for hiding this comment

NMinhNguyen commented Apr 17, 2018 •

edited

treshugart commented Apr 20, 2018 •

edited

treshugart commented Apr 20, 2018 •

edited

NMinhNguyen commented Apr 20, 2018 •

edited

NMinhNguyen commented Apr 21, 2018 •

edited

NMinhNguyen Apr 21, 2018 •

edited

treshugart Apr 22, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited

NMinhNguyen Apr 23, 2018 •

edited