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

Proposal to add a signal version of the "useSyncExternalStore" hook #568

Open
ndt080 opened this issue Jun 1, 2024 · 7 comments
Open

Proposal to add a signal version of the "useSyncExternalStore" hook #568

ndt080 opened this issue Jun 1, 2024 · 7 comments
Labels
enhancement New feature or request

Comments

@ndt080
Copy link

ndt080 commented Jun 1, 2024

I suggest adding an alternative version of the useSyncExternalStore hook, which will return a signal and not cause the template to be rerender. This hook will simplify the integration of state storages with preact and allow developers to use stores without rerendering components when the value of the selector changes.

Here is an example of an implementation that I wrote for my own project based on the implementation of the useSyncExternalStore hook in preact

type SyncExternalStoreInstance<T> = { _value: T; _getSnapshot: () => T };

export function is(x: any, y: any) {
	return (x === y && (x !== 0 || 1 / x === 1 / y)) || (x !== x && y !== y);
}

function didSnapshotChange<T>(inst: SyncExternalStoreInstance<T>) {
	const latestGetSnapshot = inst._getSnapshot;
	const prevValue = inst._value;
	try {
		const nextValue = latestGetSnapshot();
		return !is(prevValue, nextValue);
	} catch (error) {
		return true;
	}
}

export function useSyncExternalStoreSignal<T>(
	subscribe: (flush: () => void) => () => void,
	getSnapshot: () => T,
): ReadonlySignal<T> {
	const value = getSnapshot();

	const cache = useSignal({
		instance: { _value: value, _getSnapshot: getSnapshot },
	});

	const onDetectChanges = useCallback(
		(value: T) => {
			const { instance } = cache.peek();
			instance._value = value;
			instance._getSnapshot = getSnapshot;

			if (didSnapshotChange(instance)) {
				cache.value = { instance };
			}
		},
		[cache, getSnapshot],
	);

	useSignalEffect(() => {
		cache.value;
		onDetectChanges(getSnapshot());
	});

	useLayoutEffect(() => {
		onDetectChanges(value);
	}, [subscribe, value, getSnapshot, onDetectChanges]);

	useEffect(() => {
		const { instance } = cache.peek();

		if (didSnapshotChange(instance)) {
			cache.value = { instance };
		}

		return subscribe(() => {
			if (didSnapshotChange(instance)) {
				cache.value = { instance };
			}
		});
	}, [cache, subscribe]);

	return useComputed(() => cache.value.instance._getSnapshot());
}
@XantreDev
Copy link
Contributor

What is a use case?
You can use useComputed for this purpose, isn't it?

@rschristian
Copy link
Member

allow developers to use stores without rerendering components when the value of the selector changes.

What's the value of this? You can use any other state container (including a plain object) to achieve this.

@ndt080
Copy link
Author

ndt080 commented Jun 2, 2024

Absolutely the same use cases as the useSyncExternalStore hook https://github.com/preactjs/preact/blob/main/compat/src/index.js#L170
BUT

When using state stores (for example, zustand, nanostores, etc.) via useSyncExternalStore, your component, where the value from the store is used, will be constantly updated, as the "useState" value is returned. I suggest adding an alternative version suitable for the concept of signals - useSyncExternalStoreSignal. It returns a signal and does not rerender the component.

The above code covers two cases of status updates: if the component is being updated and if the subscription is triggered. But we can shorten the code if triggering the subscription is enough as a state update trigger.

export function useSyncExternalStoreSignal<T>(
	subscribe: (flush: () => void) => () => void,
	getSnapshot: () => T,
): ReadonlySignal<T> {
         const cache = useSignal<T>(getSnapshot());

	useLayout(() => {
		return subscribe(() => {
		            cache.value = getSnapshot();
		});
	}, [cache, subscribe, getSnapshot]);

	return useComputed(() => cache.value.instance);
}

@XantreDev
Copy link
Contributor

@rschristian I think it should be in place like recipes in docs, because use case is pretty niche.
Should we create recipes section somewhere?

@rschristian
Copy link
Member

Tbh I'm quite fine w/ leaving this as-is, I don't think we need to specifically document any of this ourselves. If someone else needs it in the future then we can link to this / move it to a discussion.


I don't think this is a good candidate for adding, as it's unlikely to be used by most. We generally have an ideology here of primitives over toolkits, and aren't looking to supply absolutely everything one can need. We focus on making sure the primitives are adapatable enough to allow structures to be build on top, as you've already done here.

For something to be added to the library, it needs to have a clear and wide value to the consumers at large, and I don't think uSESS fits the bill.

@ndt080
Copy link
Author

ndt080 commented Jun 2, 2024

Based on your logic, by what principle did the useSyncExternalStore hook get into preact/compat?
Just because React has this primitive?
In my opinion, this primitive falls under all that you have described above. It is a building block for building higher-level structures in state libraries such as useStore, useStoreSelector, etc.
I just gave a high-level implementation of the hook, but it can be implemented at a lower level to reduce the delay of translation from one signal to another and intermediate calls.

This hook fits perfectly into the concept of signals. And, as far as I thought earlier, the ideology of preact/signals is to get rid of unnecessary rerenders

@rschristian
Copy link
Member

rschristian commented Jun 2, 2024

by what principle did the useSyncExternalStore hook get into preact/compat?

preact/compat is the (sub)package that mirrors React's API. That's why it's added and why it sits there, rather than in preact/hooks.

It is a building block for building higher-level structures

It can be, but it's not an essential one. It's perfectly fine for you to find this usable or important, but generally, for something to be added into the library it has to provide value to most users.

state libraries such as useStore, useStoreSelector, etc.

You shouldn't really need any of this though. Part of the value of signals is that you can subscribe to rather granular bits of data. These old React APIs and restrictions can usually get dropped. They don't tend to make much sense w/ signals.

the ideology of preact/signals is to get rid of unnecessary rerenders

In part, sure.

I'll leave this open, but I don't think it's a good candidate for addition. Too niche.

@rschristian rschristian added the enhancement New feature or request label Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants