Skip to content

Commit

Permalink
Merge 51057f2 into 8e848b0
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewiggins committed Oct 28, 2019
2 parents 8e848b0 + 51057f2 commit e39c1fc
Show file tree
Hide file tree
Showing 5 changed files with 345 additions and 309 deletions.
28 changes: 17 additions & 11 deletions debug/src/debug.js
Expand Up @@ -3,6 +3,8 @@ import { getDisplayName } from './devtools/custom';
import { options, Component } from 'preact';
import { ELEMENT_NODE, DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE } from './constants';

const isWeakMapSupported = typeof WeakMap === 'function';

function getClosestDomNodeParent(parent) {
if (!parent) return {};
if (typeof parent.type === 'function') {
Expand All @@ -19,7 +21,11 @@ export function initDebug() {
let oldCatchError = options._catchError;
let oldRoot = options._root;
let oldHook = options._hook;
const warnedComponents = { useEffect: {}, useLayoutEffect: {}, lazyPropTypes: {} };
const warnedComponents = !isWeakMapSupported ? null : ({
useEffect: new WeakMap(),
useLayoutEffect: new WeakMap(),
lazyPropTypes: new WeakMap()
});

options._catchError = (error, vnode, oldVNode) => {
let component = vnode && vnode._component;
Expand Down Expand Up @@ -58,7 +64,7 @@ export function initDebug() {
}
if (!isValid) throw new Error(`
Expected a valid HTML node as a second argument to render.
Received ${parentNode} instead: render(<${vnode.type.name || vnode.type} />, ${parentNode});
Received ${parentNode} instead: render(<${getDisplayName(vnode)} />, ${parentNode});
`);

if (oldRoot) oldRoot(vnode, parentNode);
Expand Down Expand Up @@ -147,12 +153,12 @@ export function initDebug() {

// Check prop-types if available
if (typeof vnode.type==='function' && vnode.type.propTypes) {
if (vnode.type.displayName === 'Lazy' && !warnedComponents.lazyPropTypes[vnode.type]) {
if (vnode.type.displayName === 'Lazy' && warnedComponents && !warnedComponents.lazyPropTypes.has(vnode.type)) {
const m = 'PropTypes are not supported on lazy(). Use propTypes on the wrapped component itself. ';
try {
const lazyVNode = vnode.type();
warnedComponents.lazyPropTypes[vnode.type] = true;
console.warn(m + 'Component wrapped in lazy() is ' + (lazyVNode.type.displayName || lazyVNode.type.name));
warnedComponents.lazyPropTypes.set(vnode.type, true);
console.warn(m + 'Component wrapped in lazy() is ' + getDisplayName(lazyVNode));
}
catch (promise) {
console.warn(m + 'We will log the wrapped component\'s name once it is loaded.');
Expand Down Expand Up @@ -223,8 +229,6 @@ export function initDebug() {
});
}

if (oldDiffed) oldDiffed(vnode);

if (vnode._component && vnode._component.__hooks) {
let hooks = vnode._component.__hooks;
if (Array.isArray(hooks._list)) {
Expand All @@ -239,8 +243,8 @@ export function initDebug() {
}
if (Array.isArray(hooks._pendingEffects)) {
hooks._pendingEffects.forEach((effect) => {
if ((!effect._args || !Array.isArray(effect._args)) && !warnedComponents.useEffect[vnode.type]) {
warnedComponents.useEffect[vnode.type] = true;
if (!Array.isArray(effect._args) && warnedComponents && !warnedComponents.useEffect.has(vnode.type)) {
warnedComponents.useEffect.set(vnode.type, true);
console.warn('You should provide an array of arguments as the second argument to the "useEffect" hook.\n\n' +
'Not doing so will invoke this effect on every render.\n\n' +
'This effect can be found in the render of ' + getDisplayName(vnode) + '.');
Expand All @@ -249,8 +253,8 @@ export function initDebug() {
}
if (Array.isArray(hooks._pendingLayoutEffects)) {
hooks._pendingLayoutEffects.forEach((layoutEffect) => {
if ((!layoutEffect._args || !Array.isArray(layoutEffect._args)) && !warnedComponents.useLayoutEffect[vnode.type]) {
warnedComponents.useLayoutEffect[vnode.type] = true;
if (!Array.isArray(layoutEffect._args) && warnedComponents && !warnedComponents.useLayoutEffect.has(vnode.type)) {
warnedComponents.useLayoutEffect.set(vnode.type, true);
console.warn('You should provide an array of arguments as the second argument to the "useLayoutEffect" hook.\n\n' +
'Not doing so will invoke this effect on every render.\n\n' +
'This effect can be found in the render of ' + getDisplayName(vnode) + '.');
Expand All @@ -259,6 +263,8 @@ export function initDebug() {
}
}

if (oldDiffed) oldDiffed(vnode);

if (vnode._children != null) {
const keys = [];
for (let i = 0; i < vnode._children.length; i++) {
Expand Down
136 changes: 136 additions & 0 deletions debug/test/browser/debug-hooks.test.js
@@ -0,0 +1,136 @@
import { createElement, render, Component } from 'preact';
import {
useState,
useEffect,
useLayoutEffect,
useMemo,
useCallback
} from 'preact/hooks';
import { initDebug } from '../../src/debug';
import { act } from 'preact/test-utils';
import { setupScratch, teardown } from '../../../test/_util/helpers';

/** @jsx createElement */

initDebug();

describe('debug with hooks', () => {
let scratch;
let errors = [];
let warnings = [];

beforeEach(() => {
errors = [];
warnings = [];
scratch = setupScratch();
sinon.stub(console, 'error').callsFake(e => errors.push(e));
sinon.stub(console, 'warn').callsFake(w => warnings.push(w));
});

afterEach(() => {
(console.error).restore();
console.warn.restore();
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 {
componentWillMount() {
useState();
}

render() {
return <p>test</p>;
}
}
const fn = () =>
act(() =>
render(
<Foo>
<App />
</Foo>,
scratch
)
);
expect(fn).to.throw(/Hook can only be invoked from render/);
});

// TODO: Fix this test. It only passed before because render was never called.
// Once render is called, currentComponent is set and never unset so calls to
// hooks outside of components would still work.
it.skip('should throw an error when invoked outside of a component', () => {
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 warn for argumentless useEffect hooks', () => {
const App = () => {
const [state] = useState('test');
useEffect(() => 'test');
return <p>{state}</p>;
};
render(<App />, scratch);
expect(warnings[0]).to.match(/You should provide an array of arguments/);
render(<App />, scratch);
expect(warnings[1]).to.be.undefined;
});

it('should warn for argumentless useLayoutEffect hooks', () => {
const App = () => {
const [state] = useState('test');
useLayoutEffect(() => 'test');
return <p>{state}</p>;
};
render(<App />, scratch);
expect(warnings[0]).to.match(/You should provide an array of arguments/);
render(<App />, scratch);
expect(warnings[1]).to.be.undefined;
});

it('should not warn for argumented effect hooks', () => {
const App = () => {
const [state] = useState('test');
useLayoutEffect(() => 'test', []);
useEffect(() => 'test', [state]);
return <p>{state}</p>;
};
const fn = () => act(() => render(<App />, scratch));
expect(fn).to.not.throw();
});

it('should warn for useless useMemo calls', () => {
const App = () => {
const [people] = useState([40, 20, 60, 80]);
const retiredPeople = useMemo(() => people.filter(x => x >= 60));
const cb = useCallback(() => () => 'test');
return <p onClick={cb}>{retiredPeople.map(x => x)}</p>;
};
render(<App />, scratch);
expect(warnings.length).to.equal(2);
});

it('should warn when non-array args is passed', () => {
const App = () => {
const foo = useMemo(() => 'foo', 12);
return <p>{foo}</p>;
};
render(<App />, scratch);
expect(warnings[0]).to.match(/without passing arguments/);
});
});

0 comments on commit e39c1fc

Please sign in to comment.