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(useRowSelect): do not throw if toggleRowSelected id not found in current instance #3440

Closed
wants to merge 1 commit into from

Conversation

octatone
Copy link

Encountered an exception when a component callback held reference to a row that was deleted by the time it was executed. This PR adds a small change to bail early in the reducer if the toggleRowSelected action's row id is not found in the current instance.

@vercel
Copy link

vercel bot commented Aug 25, 2021

Someone is attempting to deploy a commit to a Personal Account owned by @tannerlinsley on Vercel.

@tannerlinsley first needs to authorize it.

Copy link

@theopak theopak left a comment

Choose a reason for hiding this comment

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

wonderful! i see the same issue when using react-table and i agree that this is a clean way to fix it

@@ -153,6 +153,7 @@ function reducer(state, action, previousState, instance) {

const handleRowById = id => {
const row = rowsById[id]
if (row === undefined) return
Copy link

Choose a reason for hiding this comment

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

from looking through some other files, i think it might be a repo convention to include the { } like this:

Suggested change
if (row === undefined) return
if (row === undefined) {
return
}

Comment on lines +387 to +389
describe('reducer', () => {
describe('toggleRowSelected', () => {
test('does not throw when row not found', () => {
Copy link

Choose a reason for hiding this comment

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

hey look at that! nice unit tests 👍

@faharmonyuser
Copy link

what's the status of this PR? is it going to be merged anytime soon?

@cduflo
Copy link

cduflo commented Dec 2, 2021

+1 we are looking to have this fixed for our implementation as well

@github-actions
Copy link

github-actions bot commented Apr 8, 2022

This pull request is being marked as stale (no activity in the last 14 days)

@github-actions github-actions bot added the Stale label Apr 8, 2022
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

This pull request has been detected as stale (no activity in the last 14 days) and automatically closed. It's very likely that your pull request has remained here this long because it introduces breaking changes to the v7 API. React Table v8 is currently in alpha (soon beta!) and already contains bug fixes, performance improvements and architectural changes that likely address the issue this pull request is meant to solve.

  • If your v7 pull request has been previously labeled as having breaking changes, please try the new v8 alpha/beta releases
  • If your v7 pull request has not already been labeled as a breaking change, open a new pull request.
  • If this was a v8 pull request and was closed by mistake, please reopen or leave a comment below.

@github-actions github-actions bot closed this Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants