Skip to content

Commit

Permalink
Use a WeakMap to track which components have warned already
Browse files Browse the repository at this point in the history
  • Loading branch information
andrewiggins committed Oct 28, 2019
1 parent f90beba commit b0c6727
Show file tree
Hide file tree
Showing 2 changed files with 12 additions and 14 deletions.
19 changes: 12 additions & 7 deletions debug/src/debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import { checkPropTypes } from './check-props';
import { getDisplayName } from './devtools/custom';
import { options, Component } from 'preact';
import { ELEMENT_NODE, DOCUMENT_NODE, DOCUMENT_FRAGMENT_NODE } from './constants';
import { createWeakMap } from './weakestOfMaps';

function getClosestDomNodeParent(parent) {
if (!parent) return {};
Expand All @@ -19,7 +20,11 @@ export function initDebug() {
let oldCatchError = options._catchError;
let oldRoot = options._root;
let oldHook = options._hook;
const warnedComponents = { useEffect: {}, useLayoutEffect: {}, lazyPropTypes: {} };
const warnedComponents = {
useEffect: createWeakMap(),
useLayoutEffect: createWeakMap(),
lazyPropTypes: createWeakMap()
};

options._catchError = (error, vnode, oldVNode) => {
let component = vnode && vnode._component;
Expand Down Expand Up @@ -147,11 +152,11 @@ 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.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;
warnedComponents.lazyPropTypes.set(vnode.type, true);
console.warn(m + 'Component wrapped in lazy() is ' + getDisplayName(lazyVNode));
}
catch (promise) {
Expand Down Expand Up @@ -237,8 +242,8 @@ export function initDebug() {
}
if (hooks._pendingEffects && 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.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 @@ -247,8 +252,8 @@ export function initDebug() {
}
if (hooks._pendingLayoutEffects && 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.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 Down
7 changes: 0 additions & 7 deletions debug/test/browser/debug-suspense.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -110,11 +110,6 @@ describe('debug with suspense', () => {
});
});

// TODO: These next 3 tests fail because debug is no longer re-initialized
// before each test. Re-initializing before each tests caused the warnedComponents
// array to be reset before each test ran. However, these new tests all share the same
// warnedComponents array meaning only the first Lazy component will get warned on
// because all Lazy components serialize to the same string.
it('should log the displayName', () => {
function MyLazyLoadedComponent() {
return <div>Hi there</div>;
Expand All @@ -138,7 +133,6 @@ describe('debug with suspense', () => {
});
});

// TODO: Fix debug so this test passes. See comment above
it('should not log a component if lazy throws', () => {
const loader = Promise.reject(new Error('Hey there'));
const FakeLazy = lazy(() => loader);
Expand All @@ -155,7 +149,6 @@ describe('debug with suspense', () => {
});
});

// TODO: Fix debug so this test passes. See comment above
it("should not log a component if lazy's loader throws", () => {
const FakeLazy = lazy(() => {
throw new Error('Hello');
Expand Down

0 comments on commit b0c6727

Please sign in to comment.