Skip to content

Commit

Permalink
backport #3964
Browse files Browse the repository at this point in the history
  • Loading branch information
JoviDeCroock committed Jan 22, 2024
1 parent be85077 commit de40f9e
Show file tree
Hide file tree
Showing 3 changed files with 114 additions and 18 deletions.
37 changes: 29 additions & 8 deletions debug/src/debug.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export function initDebug() {
useEffect: new WeakMap(),
useLayoutEffect: new WeakMap(),
lazyPropTypes: new WeakMap()
};
};
const deprecations = [];

options._catchError = (error, vnode, oldVNode) => {
Expand Down Expand Up @@ -341,15 +341,15 @@ export function initDebug() {
if (oldVnode) oldVnode(vnode);
};

options.diffed = vnode => {
options.diffed = internal => {
hooksAllowed = false;

if (oldDiffed) oldDiffed(vnode);
if (oldDiffed) oldDiffed(internal);

if (vnode._children != null) {
if (internal._children != null) {
const keys = [];
for (let i = 0; i < vnode._children.length; i++) {
const child = vnode._children[i];
for (let i = 0; i < internal._children.length; i++) {
const child = internal._children[i];
if (!child || child.key == null) continue;

const key = child.key;
Expand All @@ -358,8 +358,8 @@ export function initDebug() {
'Following component has two or more children with the ' +
`same key attribute: "${key}". This may cause glitches and misbehavior ` +
'in rendering process. Component: \n\n' +
serializeVNode(vnode) +
`\n\n${getOwnerStack(vnode)}`
serializeVNode(internal) +
`\n\n${getOwnerStack(internal)}`
);

// Break early to not spam the console
Expand All @@ -369,6 +369,27 @@ export function initDebug() {
keys.push(key);
}
}

if (internal.data && internal.data.__hooks != null) {
// Validate that none of the hooks in this component contain arguments that are NaN.
// This is a common mistake that can be hard to debug, so we want to catch it early.
const hooks = internal.data.__hooks._list;
if (hooks) {
for (let i = 0; i < hooks.length; i += 1) {
const hook = hooks[i];
if (hook._args) {
for (const arg of hook._args) {
if (Number.isNaN(arg)) {
const componentName = getDisplayName(internal);
throw new Error(
`Invalid argument passed to hook. Hooks should not be called with NaN in the dependency array. Hook index ${i} in component ${componentName} was called with NaN.`
);
}
}
}
}
}
}
};
}

Expand Down
74 changes: 74 additions & 0 deletions debug/test/browser/validateHookArgs.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { createElement, render, createRef } from 'preact';
import {
useState,
useEffect,
useLayoutEffect,
useCallback,
useMemo,
useImperativeHandle
} from 'preact/hooks';
import { setupRerender } from 'preact/test-utils';
import { setupScratch, teardown } from '../../../test/_util/helpers';
import 'preact/debug';

/** @jsx createElement */

describe('Hook argument validation', () => {
/**
* @param {string} name
* @param {(arg: number) => void} hook
*/
function validateHook(name, hook) {
const TestComponent = ({ initialValue }) => {
const [value, setValue] = useState(initialValue);
hook(value);

return (
<button type="button" onClick={() => setValue(NaN)}>
Set to NaN
</button>
);
};

it(`should error if ${name} is mounted with NaN as an argument`, async () => {
expect(() =>
render(<TestComponent initialValue={NaN} />, scratch)
).to.throw(/Hooks should not be called with NaN in the dependency array/);
});

it(`should error if ${name} is updated with NaN as an argument`, async () => {
render(<TestComponent initialValue={0} />, scratch);

expect(() => {
scratch.querySelector('button').click();
rerender();
}).to.throw(
/Hooks should not be called with NaN in the dependency array/
);
});
}

/** @type {HTMLElement} */
let scratch;
/** @type {() => void} */
let rerender;

beforeEach(() => {
scratch = setupScratch();
rerender = setupRerender();
});

afterEach(() => {
teardown(scratch);
});

validateHook('useEffect', arg => useEffect(() => {}, [arg]));
validateHook('useLayoutEffect', arg => useLayoutEffect(() => {}, [arg]));
validateHook('useCallback', arg => useCallback(() => {}, [arg]));
validateHook('useMemo', arg => useMemo(() => {}, [arg]));

const ref = createRef();
validateHook('useImperativeHandle', arg => {
useImperativeHandle(ref, () => undefined, [arg]);
});
});
21 changes: 11 additions & 10 deletions src/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ export as namespace preact;
import { JSXInternal } from './jsx';

export import JSX = JSXInternal;
import { Internal } from './internal';

//
// Preact Virtual DOM
Expand Down Expand Up @@ -76,12 +77,11 @@ export type ComponentFactory<P = {}> = ComponentType<P>;

export type ComponentProps<
C extends ComponentType<any> | keyof JSXInternal.IntrinsicElements
> =
C extends ComponentType<infer P>
? P
: C extends keyof JSXInternal.IntrinsicElements
? JSXInternal.IntrinsicElements[C]
: never;
> = C extends ComponentType<infer P>
? P
: C extends keyof JSXInternal.IntrinsicElements
? JSXInternal.IntrinsicElements[C]
: never;

export interface FunctionComponent<P = {}> {
(props: RenderableProps<P>, context?: any): VNode<any> | null;
Expand Down Expand Up @@ -311,9 +311,9 @@ export interface Options {
/** Attach a hook that is invoked whenever a VNode is created. */
vnode?(vnode: VNode): void;
/** Attach a hook that is invoked immediately before a vnode is unmounted. */
unmount?(vnode: VNode): void;
unmount?(vnode: Internal): void;
/** Attach a hook that is invoked after a vnode has rendered. */
diffed?(vnode: VNode): void;
diffed?(vnode: Internal): void;
event?(e: Event): any;
requestAnimationFrame?(callback: () => void): void;
debounceRendering?(cb: () => void): void;
Expand Down Expand Up @@ -356,7 +356,8 @@ export interface Context<T> {
displayName?: string;
}
export interface PreactContext<T> extends Context<T> {}
export type ContextType<C extends Context<any>> =
C extends Context<infer T> ? T : never;
export type ContextType<C extends Context<any>> = C extends Context<infer T>
? T
: never;

export function createContext<T>(defaultValue: T): Context<T>;

0 comments on commit de40f9e

Please sign in to comment.