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

WIP - Exploring using search from Rails #18

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

WIP - Exploring using search from Rails #18

wants to merge 1 commit into from

Conversation

arv
Copy link

@arv arv commented Mar 18, 2024

No description provided.

Copy link

render bot commented Mar 18, 2024

@arv arv changed the title WIP WIP - Exploring using search from Rails Mar 19, 2024

type ReplicacheLike = Parameters<typeof generateZQL>[0];

export function useTable<E extends Entity>(

Choose a reason for hiding this comment

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

A few things:

  • Is this a hook? use is typically reserved for hooks.
  • I don't think we'll call them tables, probably collections.
  • What about query<T>(rep, name)
  • Also I think it is fine to wrap this all up into a useQuery react hook and not expose useTable/query at all. I bet it will feel more natural to just repeat shared sections of the query rather than share them. They're so succint already.

Choose a reason for hiding this comment

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

useTable does feel redundant.

Copy link
Author

Choose a reason for hiding this comment

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

I initially had a useMemo in there... that is why it was structured like a hook.

Copy link
Author

@arv arv Mar 20, 2024

Choose a reason for hiding this comment

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

Also I think it is fine to wrap this all up into a useQuery react hook and not expose useTable/query at all. I bet it will feel more natural to just repeat shared sections of the query rather than share them. They're so succint already.

The query starts with an object... If we want to collapse this into one hook it needs to be something along the lines of:

useQuery<List[]>(rep, 'list', q => q.select('id', 'name').where(...), deps);

This looks fine to me :-)

But we also need to get parse in there...

and when there is join involved we need to register all the key namespaces.

useQuery(rep, {
  list: listSchema.parse,
  todo: todoSchema.parse,
}, q => ..., deps)

query: EntityQuery<S, R>,
dependencies: unknown[] = [],
): R {
const view = useMemo(() => query.prepare().materialize(), dependencies);

Choose a reason for hiding this comment

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

you'll need a way to destroy the statement.

): R {
const view = useMemo(() => query.prepare().materialize(), dependencies);
const [value, setValue] = useState<R>(view.value as R);
useEffect(() => {
Copy link

@tantaman tantaman Mar 20, 2024

Choose a reason for hiding this comment

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

Setting state via useEffect causes UI flashing / tearing.

useState calls inside useEffect will not run in the same render pass as the render caused by a change in dependencies.

In other words, the component will render and fully paint to the browser once with the updated dependencies and then again with the effects (setValue) of useEffect.

You'd need to emulate useEffect with useState to prevent flashing.

const [prevDependencies, setPrevDependencies] = useState(dependencies);
if (!shallowEqual(dependencies, prevDependencies)) {
  setPrevDependencies(prevDependencies);
  const newView = viewFn();
  setValue(newView.value);
  setView(newView);
  disposer();
  setDisposer(newView.on(setValue));
}

This will ensure that when dependencies change, the new view is created and value set on the component before it renders to the screen.

some related discussion: https://twitter.com/tantaman/status/1732140032729985512

Copy link
Author

Choose a reason for hiding this comment

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

You have certainly opened up a can of worms!

The code that you have here and the code on twitter seems incomplete. We need to have a cleanup phase too to remove the listener.

Choose a reason for hiding this comment

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

See also useLayoutEffect ?

Choose a reason for hiding this comment

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

The code that you have here and the code on twitter seems incomplete. We need to have a cleanup phase too to remove the listener.

setDisposer would handle that, although given the disposer isn't used to render anything it should be a ref rather than state.

This should be more complete:

// track previous deps
const [prevDependencies, setPrevDependencies] = useState();
// track our view / query results
const [view, setView] = useState();
// track our subscription to the view
const [disposer, setDisposer] = useState();

// prevDeps === undefined -> first render
// prevDeps != deps, re-render
if (prevDependencies === undefined || !shallowEqual(dependencies, prevDependencies)) {
  setPrevDependencies(prevDependencies);
  // new deps? compute view / query
  const newView = viewFn();
  // destroy old view
  view?.destroy();
  // destroy old subscription
  disposer?.();

  setValue(newView.value);
  setView(newView);
  setDisposer(newView.on(setValue));
}

// also need to try cleaning up on un-mount of the component
useEffect(() => {
  disposer?.();
  view?.destroy();
}, []);

Copy link
Author

Choose a reason for hiding this comment

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

Thanks. I will try again to see if I can get something along these lines working.

For replicache-react, useSubscribe was simpler because useSubscribe is always async and we do not have the result immediately.

Copy link

@tantaman tantaman left a comment

Choose a reason for hiding this comment

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

see inline comments

.desc('sort');
const todos = useQuery(todosQuery, [listID]);

const todosCount = useQuery(todoTable.where('listID', '=', listID).count());
Copy link
Author

Choose a reason for hiding this comment

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

missing [listID] deps

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants