Merge pull request #2816 from preactjs/invalid-hook-throw

debug/test/browser/debug-hooks.test.js

@@ -25,19 +25,18 @@ describe('debug with hooks', () => {
 		teardown(scratch);
 	});
 
-	// TODO: Fix this test. It only passed before because App was the first component
-	// into render so currentComponent in hooks/index.js wasn't set yet. However,
-	// any children under App wouldn't have thrown the error if they did what App
-	// did because currentComponent would be set to App.
-	// In other words, hooks never clear currentComponent so once it is set, it won't
-	// be unset
-	it.skip('should throw an error when using a hook outside a render', () => {
-		const Foo = props => props.children;
-		class App extends Component {
+	it('should throw an error when using a hook outside a render', () => {
+		class Foo extends Component {
 			componentWillMount() {
 				useState();
 			}
 
+			render() {
+				return this.props.children;
+			}
+		}
+
+		class App extends Component {
 			render() {
 				return <p>test</p>;
 			}
@@ -81,6 +80,20 @@ describe('debug with hooks', () => {
 		expect(fn).to.throw(/Hook can only be invoked from render/);
 	});
 
+	it('should throw an error when invoked outside of a component after render', () => {
+		function Foo(props) {
+			useEffect(() => {}); // Pretend to use a hook
+			return props.children;
+		}
+
+		const fn = () =>
+			act(() => {
+				render(<Foo>Hello!</Foo>, scratch);
+				useState();
+			});
+		expect(fn).to.throw(/Hook can only be invoked from render/);
+	});
+
 	it('should throw an error when invoked inside an effect callback', () => {
 		function Foo(props) {
 			useEffect(() => {

hooks/src/index.js

@@ -5,13 +5,21 @@ let currentIndex;
 
 /** @type {import('./internal').Component} */
 let currentComponent;
+/**
+ * Keep track of the previous component so that we can set
+ * `currentComponent` to `null` and throw when a hook is invoked
+ * outside of render
+ * @type {import('./internal').Component}
+ */
+let previousComponent;
 
 /** @type {number} */
 let currentHook = 0;
 
 /** @type {Array<import('./internal').Component>} */
 let afterPaintEffects = [];
 
+let oldBeforeDiff = options._diff;
 let oldBeforeRender = options._render;
 let oldAfterDiff = options.diffed;
 let oldCommit = options._commit;
@@ -20,6 +28,11 @@ let oldBeforeUnmount = options.unmount;
 const RAF_TIMEOUT = 100;
 let prevRaf;
 
+options._diff = vnode => {
+	currentComponent = null;
+	if (oldBeforeDiff) oldBeforeDiff(vnode);
+};
+
 options._render = vnode => {
 	if (oldBeforeRender) oldBeforeRender(vnode);
 
@@ -41,6 +54,7 @@ options.diffed = vnode => {
 	if (c && c.__hooks && c.__hooks._pendingEffects.length) {
 		afterPaint(afterPaintEffects.push(c));
 	}
+	currentComponent = previousComponent;
 };
 
 options._commit = (vnode, commitQueue) => {