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

fix: Memoize use_table_data listener #428

Merged
merged 6 commits into from
Apr 30, 2024

Conversation

jnumainville
Copy link
Collaborator

@jnumainville jnumainville commented Apr 19, 2024

Because the listener function was not memoized in use_table_data, the use_effect within use_table_listener was getting called unnecessarily, which means in some cases table updates can be missed during the start and stop of the listener.

Added fix and caution message in use_table_listener for now.

It might be worthwhile making use_table_listener more robust in cases where arguments changed, but the table specifically did not. This is not trivial though.

@jnumainville jnumainville requested a review from mofojed April 19, 2024 17:25
@jnumainville jnumainville self-assigned this Apr 19, 2024
@jnumainville
Copy link
Collaborator Author

I was hoping this would fix those test issues, but I suppose not. Technically the check that is breaking isn't necessary but I want to figure out why it's breaking in this way.

@jnumainville
Copy link
Collaborator Author

Broken test was a multithreading issue. I've added some utilities to the hook testing for these cases.

@jnumainville jnumainville requested a review from mofojed April 22, 2024 20:17
@jnumainville jnumainville requested a review from mofojed April 26, 2024 20:22
@jnumainville jnumainville merged commit f342dad into deephaven:main Apr 30, 2024
13 checks passed
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.

2 participants