Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support ref cleanup functions #4436

Merged
merged 2 commits into from
Jul 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 15 additions & 3 deletions src/diff/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -554,14 +554,26 @@ function diffElementNodes(

/**
* Invoke or update a ref, depending on whether it is a function or object ref.
* @param {Ref<any>} ref
* @param {Ref<any> & { _unmount?: unknown }} ref
* @param {any} value
* @param {VNode} vnode
*/
export function applyRef(ref, value, vnode) {
try {
if (typeof ref == 'function') ref(value);
else ref.current = value;
if (typeof ref == 'function') {
let hasRefUnmount = typeof ref._unmount == 'function';
Copy link
Member

Choose a reason for hiding this comment

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

We'll need to add _unmount to mangle to be safe I reckon

Copy link
Member Author

Choose a reason for hiding this comment

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

It's already there as we use that for options._unmount too.

if (hasRefUnmount) {
// @ts-ignore TS doesn't like moving narrowing checks into variables
ref._unmount();
}

if (!hasRefUnmount || value != null) {
Copy link
Member

@JoviDeCroock JoviDeCroock Jul 7, 2024

Choose a reason for hiding this comment

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

Why do we need !hasRefUnmount here? Wouldn't it be safe to always invoke this and inline typeof ref._unmount == 'function' above?

EDIT: seems needed when I tested it out

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe inverting the check makes it more clear. When a cleanup function is returned, then the ref function should never be invoked with null. But when no cleanup function is there it should be invoked with null.

// Store the cleanup function on the function
// instance object itself to avoid shape
// transitioning vnode
ref._unmount = ref(value);
}
} else ref.current = value;
} catch (e) {
options._catchError(e, vnode);
}
Expand Down
5 changes: 4 additions & 1 deletion src/internal.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,10 @@ declare global {
// We use the `current` property to differentiate between the two kinds of Refs so
// internally we'll define `current` on both to make TypeScript happy
type RefObject<T> = { current: T | null };
type RefCallback<T> = { (instance: T | null): void; current: undefined };
type RefCallback<T> = {
(instance: T | null): void | (() => void);
current: undefined;
};
type Ref<T> = RefObject<T> | RefCallback<T>;

export interface VNode<P = {}> extends preact.VNode<P> {
Expand Down
32 changes: 32 additions & 0 deletions test/browser/refs.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -715,4 +715,36 @@ describe('refs', () => {
render(<App />, scratch);
render(<App show />, scratch);
});

it('should call ref cleanup on unmount', () => {
const cleanup = sinon.spy();
const ref = sinon.spy(() => cleanup);

function App({ show = false }) {
return <div>{show && <p ref={ref}>hello</p>}</div>;
}

render(<App show />, scratch);
render(<App />, scratch);

expect(cleanup).to.be.calledOnce;

// Ref should not be called with `null` when cleanup is present
expect(ref).to.be.calledOnce;
expect(ref).not.to.be.calledWith(null);
});

it('should call ref cleanup when ref changes', () => {
const cleanup = sinon.spy();

function App({ show = false, count = 0 }) {
return <div>{show && <p ref={() => cleanup}>hello {count}</p>}</div>;
}

render(<App show />, scratch);
render(<App show count={1} />, scratch);

// Cleanup should be invoked whenever ref function changes
expect(cleanup).to.be.calledOnce;
});
});
Loading